http: Improve HTTPRemoteClient::MaybeDisconnect() #36159

hodlinator commented at 10:41 AM on September 3, 2026: contributor

Issues

#35182 had 1 case where we flag a client for disconnect where we do not log the reason.

#35829 introduced the somewhat poorly named non-const HTTPRemoteClient::MaybeDisconnect(). It also lacks testing.

The control-flow in MaybeDisconnect()/ShouldDisconnect() is somewhat complex. #35829 (review)

Commits solving these issues in same order

http: Add log message for remaining case where we set HTTPRemoteClient::m_disconnect = true
refactor: Rename MaybeDisconnect() to ShouldDisconnect() and make it const
refactor: Enable mocking the time around ShouldDisconnect()
test: Add test for behavior of ShouldDisconnect()
refactor: Flatten control flow in ShouldDisconnect()

http: Add log message for remaining case where we set m_disconnect = true a7ffe7acba

refactor: Rename MaybeDisconnect() to ShouldDisconnect() and make it const

Also reins in whitespace while touching the lambda ("Maybe" is one char shorter than "Should" so aligning whitespace would happen anyway).

b0e7b0cc52

DrahtBot added the label RPC/REST/ZMQ on Sep 3, 2026

DrahtBot commented at 10:41 AM on September 3, 2026: contributor

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

Code Coverage & Benchmarks

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

Reviews

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

Type	Reviewers
ACK	winterrdog
Concept ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35713 (Remove boost as a unit test runner by rustaceanrob)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

ZeroSock::Send() claiming its able to send everything -> ZeroSock::Send() claiming it's able to send everything [“its” should be “it’s”]

2026-09-05 20:32:34

jeanpablojp commented at 2:19 PM on September 4, 2026: contributor

Concept ACK

Built the four commits and mutation-tested the changed branches.

now is a whole second, taken before the clients exist and compared against an m_idle_since they stamp later, so the checks expecting a disconnect at now + timeout + 1s only hold while both fall in the same second. About 0.6ms sits in between on busy_slow, and a rollover there makes the comparison 30s > 30s. Forced one to confirm. Worth taking now per block, after WriteReply() for busy_slow?

refactor: Switch client idle timeout to mockable clock

Enables precision in later tests.

9afe15897e

test: Verify behavior of HTTPRemoteClient::ShouldDisconnect()

Best to nail this down before the refactor in the next commit.

Co-authored-by: JP <jeanpablo.jp@hotmail.com>

784ac4fb9b

http: Flatten control flow in HTTPRemoteClient::ShouldDisconnect()

There's a slight behavior change in that clients already flagged with m_disconnect = true no longer also get logged as passing the idle timeout if that condition would also be met.

Co-authored-by: winterrdog <winterrdog@protonmail.ch>

352fdc2797

in src/test/httpserver_tests.cpp:857 in 332f4e48c5

 852 | +            }
 853 | +        };
 854 | +
 855 | +        // Allow disconnecting directly after WriteReply() when it is able to
 856 | +        // complete the optimistic send of the full response due to the FastSocket.
 857 | +        auto busy_fast{std::make_shared<Client>(std::make_unique<FastSocket>())};

jeanpablojp commented at 2:19 PM on September 4, 2026:

ZeroSock::Send() already returns len, so could this just be a ZeroSock?

        // Allow disconnecting directly after WriteReply() when it is able to
        // complete the optimistic send of the full response due to the ZeroSock.
        auto busy_fast{std::make_shared<Client>(std::make_unique<ZeroSock>())};

in src/httpserver.cpp:1115 in 332f4e48c5 outdated

1118 | +bool HTTPRemoteClient::ShouldDisconnect(std::chrono::time_point<SteadyClock> now, std::chrono::seconds rpcservertimeout, bool disconnect_all) const
1119 |  {
1120 | -    // First check for idle timeout. We reset the timer when we send and receive data,
1121 | +    // Disconnect this client due to error or end of communication
1122 | +    // May drop unsent data if we are closing due to error.
1123 | +    if (m_disconnect) {

jeanpablojp commented at 2:19 PM on September 4, 2026:

This also stops the idle line being logged when the client is already flagged. Before, is_idle was worked out first and logged inside if (m_disconnect || is_idle), so a client already flagged and past the timeout got both lines. Looks like an improvement, since that line was claiming a timeout for a client leaving for another reason. Worth a mention in the commit message, since it says refactor:?

in src/test/httpserver_tests.cpp:798 in 332f4e48c5 outdated

 793 | +    {
 794 | +        // Nothing to disconnect for yet.
 795 | +        auto idle{std::make_shared<Client>()};
 796 | +        BOOST_CHECK(!idle->ShouldDisconnect(now, timeout, /*disconnect_all=*/false));
 797 | +        // A shutdown waits for a connection that is still busy, and it starts busy.
 798 | +        BOOST_CHECK(!idle->ShouldDisconnect(now, timeout, /*disconnect_all=*/true));

jeanpablojp commented at 2:19 PM on September 4, 2026:

A connection that is accepted and never sends anything keeps m_connection_busy at its initial true, so the disconnect_all branch never picks it up. Inside ShouldDisconnect() only the idle timeout is left, and with -rpcservertimeout=0 there is no idle timeout either. On regtest shutdown goes from 0.7s to 30.7s and ends in a Force-disconnecting warning. At the default of 30s that timeout and StopHTTPServer()'s own deadline are the same number. Releasing them at once would give up the 503 a client can still get today by sending after stop. Is the wait deliberate?

hodlinator commented at 7:34 PM on September 5, 2026:

re #36159 (review): You are pointing out behavior on master, right? Maybe we could disconnect clients quicker if they just open the connection and don't send anything, within 5 seconds or so? Not sure I want to increase the complexity of this PR though. That would probably introduce another timeout-setting.

jeanpablojp commented at 10:27 PM on September 5, 2026:

Yes, master, but also measured at the merge-base and on today's master, the same both times, under a second to shut down with no such connection open and 30.7s with one at -rpcservertimeout=0.

The slots are the sharper half. With -rpcmaxconnections=4 and four sockets that connect and send nothing, a normal call has to wait for the idle timeout to free one, and -rpcservertimeout=0 means nothing frees them. Agreed it does not belong here though, and your five seconds sounds like the right direction. Happy to open a follow-up.

in src/test/httpserver_tests.cpp:869 in 332f4e48c5 outdated

 864 | +        }
 865 | +        BOOST_CHECK( busy_fast->ShouldDisconnect(now + timeout - 1s, timeout, /*disconnect_all=*/false));
 866 | +    }
 867 | +
 868 | +    {
 869 | +        // A malformed request flags the client, and that outranks the rest.

jeanpablojp commented at 2:19 PM on September 4, 2026:

Every client here is HTTP/1.0, so m_keep_alive is never true and the branch for a healthy persistent connection is only ever asserted not firing. Replacing that condition with false leaves this test green. Worth one more client?

    {
        // A keep-alive client whose reply is out is neither flagged nor idle,
        // so the shutdown branch is the only thing that can release it.
        auto keepalive{std::make_shared<Client>(std::make_unique<ZeroSock>())};
        keepalive->MutateRecvBuffer().append("GET / HTTP/1.1\nHost: 127.0.0.1\n\n");
        {
            std::unique_ptr request{HTTPRemoteClient::TryReadRequest(keepalive)};
            BOOST_REQUIRE(request);
            request->WriteReply(HTTP_OK);
        }
        const auto now{Now<SteadySeconds>()};
        BOOST_CHECK(!keepalive->ShouldDisconnect(now, timeout, /*disconnect_all=*/false));
        BOOST_CHECK( keepalive->ShouldDisconnect(now, timeout, /*disconnect_all=*/true));
    }

    {
        // A malformed request flags the client, and that outranks the rest.

hodlinator force-pushed on Sep 5, 2026

hodlinator commented at 8:39 PM on September 5, 2026: contributor

Thanks for the feedback @jeanpablojp (https://github.com/bitcoin/bitcoin/pull/36159#pullrequestreview-5114081037)!

Addressed all of it to some extent in the latest push (352fdc279751694348c6b406322de5582f59ad49) + inline comment. Opted for mocking the clock and freeze it rather than trying to keep up with the current time of the actual clock as it drifts throughout the added test case.

DrahtBot added the label CI failed on Sep 5, 2026

DrahtBot commented at 9:31 PM on September 5, 2026: contributor

🚧 At least one of the CI tasks failed. Task MSan: https://github.com/bitcoin/bitcoin/actions/runs/33990407720/job/101371509013 LLM reason (✨ experimental): CI failed during the build link step (bitcoin-wallet) due to a ld.bfd error: “bad reloc symbol index … in section .debug_addr,” causing the final link to fail.

<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>

winterrdog commented at 12:19 AM on September 6, 2026: contributor

tACK 352fdc279751694348c6b406322de5582f59ad49

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

P.S: the CI failure is completely independent of the changes introduced by this PR

DrahtBot requested review from jeanpablojp on Sep 6, 2026