http: linger-close after parse errors so clients can read the reply #35780

pull b-l-u-e wants to merge 2 commits into bitcoin:master from b-l-u-e:http-lingering-close changing 8 files +139 −21
  1. b-l-u-e commented at 9:52 PM on July 22, 2026: contributor

    Based on this issue #35632 raised i believe the issue is not with whitespace header validation since the logic works fine coz the server detects the malformed request and queues a 400 reply as expected.

    The problem is how the server closes the connection afterwards. It queues the reply and closes the socket right away sometimes before it has finished reading everything the client had sent. So when socket is closed while there's still unread data sitting in the kernel's receive buffer, Windows responds with a TCP RST. That reset just throws away the reply that was already on its way out, so the client never gets to read the 400 and instead sees the connection aborted.

    So my LLM Buddie and I realized that we should change how the server closes a connection after a parse/size error so the client always gets a chance to read the response first by queuing the 400/413 response with Connection: close. Then, waits until the response has actually left the send buffer. Then, Half-close only the write side (shutdown(SHUT_WR/SD_SEND)) so the client can still finish reading while the server stops sending. Afterwards, Keep draining whatever the client is still sending until it hits EOF or a socket error and only force the connection closed after a 1s fallback timeout, for a client that never finishes.

  2. DrahtBot added the label RPC/REST/ZMQ on Jul 22, 2026
  3. DrahtBot commented at 9:52 PM on July 22, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35780.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    Concept ACK pinheadmz, frankomosh, winterrdog, hodlinator

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35829 (http: Make class fields private and make HTTPResponse a struct by hodlinator)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. b-l-u-e marked this as a draft on Jul 23, 2026
  5. b-l-u-e renamed this:
    http: linger-close after parse errors so clients can read the reply
    [WIP] http: linger-close after parse errors so clients can read the reply
    on Jul 23, 2026
  6. b-l-u-e renamed this:
    [WIP] http: linger-close after parse errors so clients can read the reply
    http: linger-close after parse errors so clients can read the reply
    on Jul 28, 2026
  7. b-l-u-e force-pushed on Jul 28, 2026
  8. b-l-u-e force-pushed on Jul 28, 2026
  9. DrahtBot added the label CI failed on Jul 28, 2026
  10. b-l-u-e force-pushed on Jul 28, 2026
  11. DrahtBot removed the label CI failed on Jul 28, 2026
  12. b-l-u-e marked this as ready for review on Jul 29, 2026
  13. pinheadmz commented at 5:26 PM on July 29, 2026: member

    Is there any way to cover the new behavior with functional tests? For example, maybe in interface_http can assertions be added to the try/except blocks? Like, does this PR eliminate those race conditions?

  14. in src/httpserver.cpp:543 in 2f14e212e8 outdated
     539 | @@ -512,7 +540,7 @@ bool HTTPRequest::LoadBody(LineReader& reader)
     540 |      }
     541 |  }
     542 |  
     543 | -void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body)
     544 | +void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body, bool force_close)
    


    pinheadmz commented at 5:29 PM on July 29, 2026:

    I'm not sure why we need a flag force_close in addition to HTTPRemoteClient.m_disconnect?


    b-l-u-e commented at 5:05 PM on July 30, 2026:

    the force_close is about HTTP response, send connection:close and dont keep-alive then for m_disconnect is for tear down this socket now .. so we need force_close first then m_disconnect later in order to tell client we are done flush the error reply drain a little bit then close so if we set m_disconnect immediately will close too early that will hit RST and hide the reply

  15. in src/httpserver.cpp:931 in 2f14e212e8 outdated
     931 | +                if (!lingering_close) {
     932 | +                    // Copy data from socket buffer to client receive buffer
     933 | +                    client->m_recv_buffer.insert(
     934 | +                        client->m_recv_buffer.end(),
     935 | +                        buf,
     936 | +                        buf + nrecv);
    


    pinheadmz commented at 5:31 PM on July 29, 2026:

    If we're just draining the socket and ignoring the data anyway, do we still need to copy it into memory at all? (And then clear it immediately?)


    b-l-u-e commented at 5:07 PM on July 30, 2026:

    i agree i will clean it up

  16. in src/httpserver.cpp:1081 in 2f14e212e8 outdated
    1077 | @@ -1045,6 +1078,7 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemot
    1078 |  void HTTPServer::DisconnectClients()
    1079 |  {
    1080 |      const auto now{Now<SteadySeconds>()};
    1081 | +    const auto now_ms{Now<SteadyMilliseconds>()};
    


    pinheadmz commented at 5:33 PM on July 29, 2026:

    I think if we need this level of accuracy here, we can just replace the seconds counter with ms and only use one. See #35182 (review)


    b-l-u-e commented at 5:07 PM on July 30, 2026:

    agreed will update

  17. in src/httpserver.cpp:283 in 2f14e212e8 outdated
     278 | + * Covers requests rejected while parsing: malformed syntax as well as an
     279 | + * oversized body. Runs only on the HTTP I/O thread. The send path starts the
     280 | + * timeout and half-closes after flushing the reply; the receive path then
     281 | + * drains to EOF.
     282 | + */
     283 | +static void SendErrorReplyAndLingerClose(HTTPRequest& req, HTTPRemoteClient& client, HTTPStatusCode status)
    


    pinheadmz commented at 5:34 PM on July 29, 2026:

    I don't totally understand why we need a specific function for this, like a linger close is a special type of close? Why don't we just make it so every client close (for any reason) drains the incoming socket for up to 1 second first?


    b-l-u-e commented at 5:07 PM on July 30, 2026:

    well linger isnt needed for every close its useful when we reject a request while client is still sending huge body or bad request so if we close with unread data still sitting there then windows can RST and client never sees 400 or 413

  18. in src/httpserver.cpp:939 in 2f14e212e8 outdated
     940 |          // Process as much received data as we can.
     941 |          // This executes for every client whether or not reading or writing
     942 |          // took place because it also (might) parse a request we have already
     943 |          // received and pass it to a worker thread.
     944 | -        MaybeDispatchRequestsFromClient(client);
     945 | +        if (!lingering_close) {
    


    pinheadmz commented at 5:36 PM on July 29, 2026:

    I don't think we need to guard this, since a lingering close happens in the middle of a request being processed already. This will be even better handled after #35735 when we won't even read from the socket if one request is being handled.


    b-l-u-e commented at 5:09 PM on July 30, 2026:

    i thinkk we still want the guard because once we are lingering that connection should only drain and go away not keep parsing or dispatching more work in #35735 helps with one request at time less queue weirdnes but doesnt remove the need for if we are shutting this connection via linger doesnt treat it as a normal live client

  19. pinheadmz commented at 5:38 PM on July 29, 2026: member

    Concept ACK on lingering close to be more polite to clients and improve test determinism. However I am not convinced about the approach. I also think this is a feature that would be easier to implement after #35735

  20. b-l-u-e commented at 5:09 PM on July 30, 2026: contributor

    Concept ACK on lingering close to be more polite to clients and improve test determinism. However I am not convinced about the approach. I also think this is a feature that would be easier to implement after #35735

    i will wait for your PR to be merged first

  21. fanquake marked this as a draft on Aug 10, 2026
  22. fanquake commented at 9:13 AM on August 10, 2026: member

    I've moved this to draft for now. In the mean time, @b-l-u-e if you want to rebase this on top of #35735, that could be worthwhile.

  23. DrahtBot added the label Needs rebase on Aug 10, 2026
  24. in src/httpserver.cpp:1097 in 2f14e212e8 outdated
    1093 | +                                            client->m_lingering_half_closed &&
    1094 | +                                            now_ms >= client->m_lingering_close_deadline.load()};
    1095 |  
    1096 | -                                        // Disconnect this client due to error, end of communication, or idle timeout.
    1097 | +                                        // Disconnect on error, EOF, idle timeout, or a stalled lingering close.
    1098 |                                          // May drop unsent data if we are closing due to error.
    


    pinheadmz commented at 5:39 PM on August 11, 2026:

    I think the motivation behind this PR is to be able to honestly remove this comment? Even in case of an error, we shouldn't drop the response for the client.

  25. frankomosh commented at 7:18 PM on August 12, 2026: contributor

    Concept ACK. Agree the goal here is worth solving

  26. 151henry151 commented at 1:27 AM on August 13, 2026: contributor

    Is there any way to cover the new behavior with functional tests? For example, maybe in interface_http can assertions be added to the try/except blocks? Like, does this PR eliminate those race conditions?

    The try/excepts in check_excessive_request_size and check_chunked_transfer are the 413 cases, where the client is still uploading when we reject. Drain-until-EOF is meant for that, and on loopback it probably does make those reliable. I still wouldn't turn them into hard asserts — if the upload hasn't finished by the fallback timeout, that's a flake that only shows up on slow runners.

    The #35632 flake is the other way around: we reject at the headers and close with the body still unread, so Windows RST's and the 400 never arrives. check_whitespace_in_headers already asserts 400.

  27. winterrdog commented at 9:55 PM on August 18, 2026: contributor

    concept ACK

    from #35780#pullrequestreview-4811176585 :

    However I am not convinced about the approach. I also think this is a feature that would be easier to implement after #35735

    fully agree with this comment


    my rough idea of how i think it can be approached:

    if rebased on #35735: once m_req->m_state transitions to an HTTPRequest::State::Error state, we know HTTP processing for that connection is all over. so, queue the 400/413, stop buffering or parsing further input, let the existing send path flush out the response, then half-close the send side and simply discard incoming data from the kernel buffer until EOF or an error or a timeout, giving well-behaved clients room to disconnect. in other words, the request state machine handles the HTTP side, and only minimal connection-level state is needed for the final TCP drain (kernel-side)

  28. util: add Sock::ShutdownSend() to half-close the send side
    Wrap shutdown(2) (SHUT_WR, or SD_SEND on Windows) behind the Sock interface so it can be overridden by the mock sockets used in the unit and fuzz tests.
    c31ea96456
  29. b-l-u-e force-pushed on Aug 24, 2026
  30. DrahtBot added the label CI failed on Aug 24, 2026
  31. DrahtBot commented at 9:58 AM on August 24, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task fuzzer,address,undefined,integer: https://github.com/bitcoin/bitcoin/actions/runs/32712722216/job/97387398955</sub> <sub>LLM reason (✨ experimental): CI failed due to a C++ build error: httpserver.cpp uses an undeclared identifier req (clang++ compile error).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  32. http: linger-close after parse errors so clients can read the reply
    When the HTTP server rejects a request during parsing a malformed header or an oversized body, it replied and then immediately closed the connection. On Windows, closing a socket that still has unread inbound data can trigger a TCP RST, which discards the queued reply before the client reads it, so the client sees a connection reset instead of the 400/413 response (see #35632).
    Instead of closing abruptly, flush the error reply, half-close the send side, and drain any remaining inbound data until the peer sends EOF. A 1s LINGERING_CLOSE_TIMEOUT bounds clients that never close their side.
    b14649f9fe
  33. b-l-u-e force-pushed on Aug 24, 2026
  34. DrahtBot removed the label Needs rebase on Aug 24, 2026
  35. DrahtBot removed the label CI failed on Aug 24, 2026
  36. DrahtBot added the label Needs rebase on Aug 26, 2026
  37. DrahtBot commented at 11:19 AM on August 26, 2026: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

  38. in src/httpserver.cpp:186 in b14649f9fe
     192 | @@ -183,7 +193,6 @@ static void MaybeDispatchRequestToWorker(std::shared_ptr<HTTPRequest> hreq)
     193 |                  err_msg = "unknown error";
     194 |              }
     195 |              // Reply so the client doesn't hang waiting for the response.
     196 | -            req->WriteHeader("Connection", "close");
    


    hodlinator commented at 2:26 PM on August 27, 2026:

    Why not write this, seems fine to have the other side close on us?

  39. hodlinator commented at 2:37 PM on August 27, 2026: contributor

    Concept ACK b14649f9fead1bbb75e035148672f376912b1835

    Thanks for working on this! Since I'm to blame for the merge conflict, here's a suggestion for how to rebase: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/35780_rebased


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-09-04 07:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me