http: Make `HTTPRequest` update state internally #36124

pull hodlinator wants to merge 3 commits into bitcoin:master from hodlinator:2026/08/http_refactors changing 4 files +115 −143
  1. hodlinator commented at 7:58 AM on August 31, 2026: contributor

    Issues in order of importance

    #35829 made the fields of HTTPRequest & HTTPRemoteClient private, but still allows anyone with a reference to a request to update the internal state enum. #35829 (review)

    HTTPRemoteClient::TryReadRequest() only needs to be a static method in order to forward a shared_ptr of a client to a new request.

    Commits solving these issues in same order

    • http: Remove HTTPRequest::SetState() and introduce HTTPRequest::Load() - Removes the ability for other classes to directly modify the request state.
    • 2 refactors: Extract HTTPRemoteClient::TryReadRequestInternal() from a combination of HTTPRemoteClient::TryReadRequest() and HTTPRemoteClient::ReadRequest() (the latter is removed)
  2. DrahtBot added the label RPC/REST/ZMQ on Aug 31, 2026
  3. DrahtBot commented at 7:59 AM on August 31, 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/36124.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    ACK winterrdog
    Concept ACK jeanpablojp
    Stale ACK janb84

    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:

    • #36160 (refactor: Minor improvements to HTTP unit tests by hodlinator)
    • #36135 (fuzz: test HTTPRequest state machine in http_request by frankomosh)

    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. jeanpablojp commented at 7:38 PM on August 31, 2026: contributor

    Concept ACK

    I ran the flattened ShouldDisconnect() against the version in master over every combination of the flags it reads, with the client inside and outside the idle window and the timeout on and off. Same return value in all 64. The only difference is that master logs the idle timeout even for a client that is already flagged for disconnect.

  5. in src/test/httpserver_tests.cpp:455 in 549cbbf67f outdated
     451 | @@ -484,50 +452,51 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests)
     452 |      public:
     453 |          DummyClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {}
     454 |  
     455 | -        void receive(std::string_view s)
     456 | +        void Receive(std::string_view s)
    


    jeanpablojp commented at 7:38 PM on August 31, 2026:

    The unit tests run ShouldDisconnect() but assert nothing about its first two branches. I broke each one and the suite stayed green both times. interface_http.py does reach both, but it blocks on the socket instead of failing. The test below covers both branches, and the new log line with them.

    <details> <summary>test for both branches and the new log line</summary>

    @@ -746,6 +746,38 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests)
         }
     }
     
    +BOOST_AUTO_TEST_CASE(http_should_disconnect_tests)
    +{
    +    struct Client : HTTPRemoteClient {
    +        Client() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {}
    +        using HTTPRemoteClient::MutateRecvBuffer;
    +    };
    +    constexpr auto timeout{30s};
    +    const auto now{Now<SteadySeconds>()};
    +
    +    // Nothing to disconnect for yet.
    +    auto idle{std::make_shared<Client>()};
    +    BOOST_CHECK(!idle->ShouldDisconnect(now, timeout, /*disconnect_all=*/false));
    +    // A shutdown waits for a connection that is still busy, and it starts busy.
    +    BOOST_CHECK(!idle->ShouldDisconnect(now, timeout, /*disconnect_all=*/true));
    +    // Past -rpcservertimeout, which a timeout of 0 disables.
    +    BOOST_CHECK(idle->ShouldDisconnect(now + timeout + 1s, timeout, /*disconnect_all=*/false));
    +    BOOST_CHECK(!idle->ShouldDisconnect(now + timeout + 1s, 0s, /*disconnect_all=*/false));
    +
    +    // A request handed to a worker holds the idle timeout off until the reply is sent.
    +    auto busy{std::make_shared<Client>()};
    +    busy->MutateRecvBuffer().append("GET / HTTP/1.0\n\n");
    +    auto request{HTTPRemoteClient::TryReadRequest(busy)};
    +    BOOST_REQUIRE(request);
    +    BOOST_CHECK(!busy->ShouldDisconnect(now + timeout + 1s, timeout, /*disconnect_all=*/false));
    +
    +    // A malformed request flags the client, and that outranks the rest.
    +    auto bad{std::make_shared<Client>()};
    +    bad->MutateRecvBuffer().append("GET / HTTP/1.0\nInvalid header with no colon\n\n");
    +    BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(bad));
    +    BOOST_CHECK(bad->ShouldDisconnect(now, 0s, /*disconnect_all=*/false));
    +}
    +
     BOOST_AUTO_TEST_CASE(http_server_socket_tests)
     {
         // Hard code the timestamp for the Date header in the HTTP response
    @@ -791,6 +823,9 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests)
         // Create a mock client with pre-loaded request data and add it to the local CreateSock queue.
         // Keep a handle for the mock client's send and receive pipes so we can examine
         // the data it "receives".
    +    // No keep-alive, so the server closes once the reply is flushed, and says so.
    +    DebugLogHelper find_close{"Done sending to client without keep-alive"};
    +
         std::shared_ptr<DynSock::Pipes> mock_client_socket_pipes{ConnectClient(std::as_bytes(std::span(full_request)))};
     
         // Wait up to a minute to find and connect the client in the I/O loop
    

    </details>


    hodlinator commented at 10:07 AM on September 2, 2026:

    Thanks! Added the checks in slightly extended form.


    hodlinator commented at 10:44 AM on September 3, 2026:

    Added co-authorship in #36159.

  6. winterrdog commented at 9:21 AM on September 1, 2026: contributor

    Concept ACK

  7. in src/httpserver.cpp:1211 in 27b5a5cdef outdated
    1217 |          case HTTPRequest::State::Error:
    1218 | -            break;
    1219 | -        }
    1220 | +            return true;
    1221 | +        } // no default case, so the compiler can warn about missing cases
    1222 |      } catch (...) {
    


    winterrdog commented at 3:58 PM on September 1, 2026:

    [!NOTE] i think NONFATAL_UNREACHABLE will do just fine. do not mind the comment below 🤝


    do you think some runtime safety like assert(false) (in debug builds) would be useful here ?

    e.g. similar to how it's done: https://github.com/bitcoin/bitcoin/blob/dc0395c5858a1d55239b82a834e5075cf2069219/src/index/blockfilterindex.cpp#L64-L71

    or here: https://github.com/bitcoin/bitcoin/blob/dc0395c5858a1d55239b82a834e5075cf2069219/src/bitcoin-cli.cpp#L430-L445

  8. hodlinator force-pushed on Sep 2, 2026
  9. in src/httpserver.cpp:1175 in 907d7342b6
    1171 | @@ -1166,47 +1172,35 @@ void HTTPServer::ClearConnectedClients()
    1172 |      m_connected.clear();
    1173 |  }
    1174 |  
    1175 | -void HTTPRemoteClient::ReadRequest(HTTPRequest& req)
    1176 | +bool HTTPRequest::Load(util::LineReader& reader)
    


    janb84 commented at 2:21 PM on September 2, 2026:

    NIT: would it not help the readability to move this function closer to the other load* helpers ? ~ L 378


    hodlinator commented at 9:07 AM on September 3, 2026:

    re #36124 (review): The order of methods in httpserver.cpp is wildly out of sync with the header. I prefer to punt on this for now, to keep the number of commits down.

  10. in src/httpserver.h:162 in 907d7342b6 outdated
     162 | -     * from a receive buffer.
     163 | -     * @param[in]   reader  A LineReader object constructed over a span of data.
     164 | -     * @returns     true    If the request field was parsed.
     165 | -     *              false   If there was not enough data in the buffer to complete the field.
     166 | +     * Try to read an HTTP request. Updates m_state.
     167 | +     * @returns     true    If the request was fully parsed or in error.
    


    janb84 commented at 3:07 PM on September 2, 2026:

    NIT. On error it does not return true. Why not make it void? non-test code already checks getstate() and doing the same in the tests makes the test slightly more strict. The bool loosy communicates 5 states in a bool

         * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/)     true    If the request was fully parsed or already complete
    

    hodlinator commented at 9:08 AM on September 3, 2026:

    re #36124 (review): Removed docstring for now as I switched to returning state which seems self-explanatory.

  11. janb84 commented at 3:08 PM on September 2, 2026: contributor

    concept ACK 907d7342b60dfa00ad81e235eccb5808b9c13da6

    The removal of the SetState() makes it a better design (imho), makes the convention based "rule" not to arbitrary set the state, to a a compiler-enforced fact

    Found a few possible NITS.

  12. in src/test/fuzz/http_request.cpp:45 in 907d7342b6


    winterrdog commented at 8:07 AM on September 3, 2026:

    nit: this comment still refers to LoadControlData/LoadHeaders/LoadBody as the call sequence above, but those were replaced by a single Load() call. since the helpers are private now too, it is not immediately clear to a reader where this logic comes from

  13. hodlinator force-pushed on Sep 3, 2026
  14. hodlinator force-pushed on Sep 3, 2026
  15. DrahtBot added the label CI failed on Sep 3, 2026
  16. hodlinator force-pushed on Sep 3, 2026
  17. hodlinator commented at 9:36 AM on September 3, 2026: contributor

    Thanks for the feedback so far! Pushes after 907d7342b60dfa00ad81e235eccb5808b9c13da6 address some nits and expands http_should_disconnect_tests to more completely verify optimistic send behavior.

  18. DrahtBot removed the label CI failed on Sep 3, 2026
  19. hodlinator force-pushed on Sep 3, 2026
  20. hodlinator commented at 10:45 AM on September 3, 2026: contributor

    Decided to split out two PRs from this one as they are quite independent. See #36159 and #36160.

  21. janb84 commented at 1:19 PM on September 3, 2026: contributor

    (re) ACK c43d544b16b61e3182467dba7355cdaa372204c6

    returning the state and not a bool at the load() function, is the better way to go imho :) Have re-reviewed the PR because of the split-off; build tested etc.

    In the commit message, is an API change a behaviour change? super micro nit. The load() documentation block has no @return anymore, is state super clear to everyone ?

  22. DrahtBot requested review from jeanpablojp on Sep 3, 2026
  23. DrahtBot requested review from winterrdog on Sep 3, 2026
  24. winterrdog commented at 8:21 PM on September 5, 2026: contributor

    is state super clear to everyone ?

    had not noticed this. i think we can add the @return with some light documentation.

    State is self-explanatory as a type, but it does not tell a caller that Load() never returns Error (it throws instead), or that the other values mean "keep calling with more data" vs "done", sth that is specific to Load()'s contract. secondly, it keeps consistency with the @throws below and the Doxygen style used elsewhere

  25. http: Extract HTTPRequest::Load() to remove SetState()
    Removes the ability for other classes to directly modify the request state.
    
    Refactor with slight API change as pre-existing Load*-methods are made private, which mainly affects unit tests.
    014c2306cc
  26. refactor: Extract HTTPRemoteClient::TryReadRequestInternal() from static HTTPRemoteClient::TryReadRequest()
    TryReadRequest() now only remains as an outer method for the sake of passing the client shared_ptr to the new request.
    e832b7dc5f
  27. refactor: Flatten HTTPRemoteClient::ReadRequest() into HTTPRemoteClient::TryReadRequestInternal() 62796e659f
  28. hodlinator renamed this:
    http: Make `HTTPRequest` update state internally, etc
    http: Make `HTTPRequest` update state internally
    on Sep 5, 2026
  29. hodlinator force-pushed on Sep 5, 2026
  30. hodlinator commented at 9:00 PM on September 5, 2026: contributor

    Latest push modifies the initial Load() commit only.

    re #36124#pullrequestreview-5102409506:

    In the commit message, is an API change a behaviour change?

    Good point, fixed.

    re #36124#pullrequestreview-5102409506 + #36124 (comment):

    is state super clear to everyone ?

    had not noticed this. i think we can add the @return with some light documentation.

    State is self-explanatory as a type, but it does not tell a caller that Load() never returns Error (it throws instead), or that the other values mean "keep calling with more data" vs "done", sth that is specific to Load()'s contract. secondly, it keeps consistency with the @throws below and the Doxygen style used elsewhere

    Added back @returns. It is true that Load() (re-)throws the first time it sets Error, but subsequent calls will return Error. The return value is only read in unit tests, so not sure if it's worth elaborating further in the doc-string.

  31. winterrdog commented at 9:24 PM on September 5, 2026: contributor

    tACK 62796e659fb79a0c6f1749677ec480a80ec26e6d

    successfully built and tested on this toolchain: Debian/clang++-18/x86_64

  32. DrahtBot requested review from janb84 on Sep 5, 2026

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-09 07:56 UTC