#36159 http: Improve HTTPRemoteClient::MaybeDisconnect()

full analysis

https://github.com/bitcoin/bitcoin/pull/36159 · hodlinator · +152/-44 in 3 files, 5 commits · labels: RPC/REST/ZMQ

Goal

  • Make HTTP client disconnection logic more robust and avoid misleading idle timeout log messages
  • Add comprehensive unit test coverage for HTTP remote client disconnection conditions

Cleans up HTTPRemoteClient disconnection handling by renaming MaybeDisconnect to a const ShouldDisconnect method and flattening its control flow. It introduces a mockable steady clock for client idle tracking, adds comprehensive unit test coverage for disconnection conditions, and logs the reason when disconnecting non-keep-alive clients.

Problem: Following the removal of libevent (#35182) and refactoring in #35829, HTTPRemoteClient::MaybeDisconnect lacked test coverage, had non-const nested control flow, missed logging for one disconnect case, and logged misleading idle timeouts for connections already flagged for termination.

Category: RPC / REST / ZMQ (#26 of 52)

P3 · test coverage

  • P3 because benefits are limited to internal HTTP server testability and minor logging corrections
  • Enables time mocking to verify client disconnect conditions without broader functional impact

Worthwhile internal cleanup and test coverage for the HTTP server stack. It fixes minor logging inaccuracies when clients disconnect and enables time mocking to thoroughly test client disconnect criteria.

Membership: Touches src/httpserver.cpp and src/httpserver.h which handle the HTTP and RPC server connection lifecycle.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1

Reviewability: Ready

  • Ready to review
  • Author addressed previous feedback and tests pass cleanly

The author addressed all reviewer comments in the latest push, tests pass cleanly, and no changes are pending.

Author status: active, addressed reviewer feedback in force push

Resolved concerns:

  • jeanpablojp noted clock drift and second-rollover issues in the unit test, which hodlinator resolved by introducing MockableSteadyClock to freeze time during the test.
  • jeanpablojp observed that clients connecting and sending nothing delay shutdown when -rpcservertimeout=0; both agreed this existing behavior is out of scope and suitable for a follow-up PR (#36204).

Agreement: Strong

  • Unanimous support with no outstanding objections
  • Concept approval and verified by mutation testing (jeanpablojp)
  • Verified by testing on Linux with clang-18 (winterrdog)

Concept ACK and mutation tested by jeanpablojp; tested ACK by winterrdog with no outstanding objections.

Both reviewers engaged with the code and confirmed the behavior improvements, with previous review questions resolved in the latest push.

  • jeanpablojp gave Concept ACK and mutation-tested the branch changes, agreeing the follow-up issue on idle connect shutdown belongs elsewhere.
  • winterrdog provided tACK on Linux/clang-18 for the head commit.

Review verdicts (DrahtBot): 1

Dependencies

Enables:

Files

112 lines under test/bench/ci.

  • src/test/httpserver_tests.cpp +112/-0
  • src/httpserver.cpp +37/-41
  • src/httpserver.h +3/-3

Card

This PR refactors HTTPRemoteClient disconnection handling by flattening the control flow in ShouldDisconnect(), making it const, adding missing debug logging when dropping non-keep-alive connections, and switching idle timeout tracking to a mockable clock. It adds dedicated unit tests covering idle timeouts, slow sends, and shutdown disconnects in httpserver_tests. Review state is clean with positive testing and no open concerns.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T17:12, confidence high, input hash 6eb5846bfa35e0ef