cli: Improve empty-response and fix -rpcclienttimeout regression #36299

fjahr commented at 2:48 PM on September 19, 2026: contributor

Two follow-ups to #34342

First commit: An empty body was not treated as a complete response. We currently check if Content-Length is greater than zero instead of whether the header was sent, so a response with a Content-Length of 0 falls into the branch for responses that carry no length and confinues to read until the peer disconnects. Our server sends an empty body with several types of errors such as a wrong RPC password but it does close the connection as well, which mitigates this from causing any serious issue. However, it would still be good to handle this correctly on the client side that we don't have to rely on the server to save us from hanging.

b-l-u-e found this in post-merge review in #34342 (review) but I didn't manage to look into it until now.

Second commit: -rpcclienttimeout no longer measures real idle time. Before the libevent removal, it used to mean give up if really nothing arrives for this long, and any newly incoming data did reset the counter. With the new code the countdown ignores progress, so a large/slow response could be cut off while data still arrives. Revert this to the old behavior.

The second commit does not have a test because I didn't manage to construct one that didn't turn out to be flaky. It may be possible but I couldn't come up with something within a scope of complexity that seems reasonable for this.

cli: Treat a response with Content-Length: 0 as complete b841ab6942

cli: Apply -rpcclienttimeout per socket wait instead of per phase 1805716354

DrahtBot added the label Scripts and tools on Sep 19, 2026

DrahtBot commented at 2:48 PM on September 19, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36299
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36299.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36299#author-ralph

Reviews

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

Type	Reviewers
ACK	0tuedon, winterrdog, hodlinator, achow101
Concept ACK	l0rinc
Stale ACK	b-l-u-e

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

Conflicts

No conflicts as of last run.

winterrdog commented at 8:54 AM on September 23, 2026: contributor

approach ACK

using optional<size_t> here clearly separates the "no Content-Length" case from the Content-Length: 0 case; previously both were represented as a plain size_t defaulting to 0, which made "absent" and "present-but-zero" indistinguishable and caused a hang, since the client would wait on the peer to close the connection even though it had already signaled an empty body

makes sense overall, as does the revert to the old timeout behaviour

0tuedon commented at 9:50 AM on September 23, 2026: none

tACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3 by copying the test_empty_response_body code and switching branch to master 248ce46faf708a832b7922ae7ef9227dbcadf0e5 and running the tests, this failed as expected, then I switched back to your branch and ran the test and it passed

<details> <summary>Log When switched to master</summary>

2026-09-22T19:07:14.647499Z TestFramework (INFO): Test that a response with Content-Length: 0 does not wait for the peer to close
2026-09-22T19:07:24.658537Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
  File "/home/User/Documents/Github/bitcoin/test/functional/test_framework/util.py", line 145, in assert_raises_process_error
    fun(*args, **kwds)
    ~~~^^^^^^^^^^^^^^^
  File "/home/User/Documents/Github/bitcoin/test/functional/test_framework/test_node.py", line 919, in __call__
    return self.cli.send_cli(self.command, *args, **kwargs)
           ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/User/Documents/Github/bitcoin/test/functional/test_framework/test_node.py", line 1006, in send_cli
    raise subprocess.CalledProcessError(returncode, p_args, output=cli_stderr)
subprocess.CalledProcessError: Command '['/home/User/Documents/Github/bitcoin/build/bin/bitcoin-cli', '-nonamed', '-datadir=/tmp/bitcoin_func_test_smywuv6t/node0', '-rpcclienttimeout=30', '-rpcconnect=127.0.0.1', '-rpcport=20073', '-rpcport=59837', '-rpcclienttimeout=10', 'echo']' returned non-zero exit status 1.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/User/Documents/Github/bitcoin/test/functional/test_framework/test_framework.py", line 145, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File "/home/User/Documents/Github/bitcoin/./build/test/functional/interface_bitcoin_cli.py", line 147, in run_test
    self.test_empty_response_body()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/User/Documents/Github/bitcoin/./build/test/functional/interface_bitcoin_cli.py", line 139, in test_empty_response_body
    assert_raises_process_error(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        1, 'Authorization failed',
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
        self.nodes[0].cli(f'-rpcport={listener.getsockname()[1]}', '-rpcclienttimeout=10').echo)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/User/Documents/Github/bitcoin/test/functional/test_framework/util.py", line 150, in assert_raises_process_error
    raise AssertionError(f"Expected substring not found in: {e.output!r}")
AssertionError: Expected substring not found in: 'error: Error while attempting to communicate with server 127.0.0.1:59837 (timeout)\n\nMake sure the bitcoind server is running and that you are connecting to the correct RPC port.\nUse "bitcoin-cli -help" for more info.\n'
2026-09-22T19:07:24.711050Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2026-09-22T19:07:24.711170Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_smywuv6t
2026-09-22T19:07:24.711212Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_smywuv6t/test_framework.log
2026-09-22T19:07:24.711308Z TestFramework (ERROR): 
2026-09-22T19:07:24.711405Z TestFramework (ERROR): Hint: Call /home/User/Documents/Github/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_smywuv6t' to consolidate all logs
2026-09-22T19:07:24.711446Z TestFramework (ERROR): 
2026-09-22T19:07:24.711483Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2026-09-22T19:07:24.711538Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2026-09-22T19:07:24.711573Z TestFramework (ERROR): 
[node 0] Cleaning up leftover process

</details>

winterrdog commented at 8:29 PM on September 23, 2026: contributor

tACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3

successfully built and tested on this toolchain: FreeBSD 15.0/clang++-19/x86_64. to me, the changes look flawless

DrahtBot requested review from 0tuedon on Sep 23, 2026

b-l-u-e commented at 9:44 AM on October 1, 2026: contributor

Tested ACK b841ab694215699cba53c4de8a7cc08b9fce259a

tested against raw listener

{ printf 'HTTP/1.1 401 Unauthorized\r\nContent-Length: 0\r\n\r\n'; sleep 120; } | nc -l 127.0.0.1 18999 &
sleep 0.3
time ./build/bin/bitcoin-cli -regtest -rpcport=18999 -rpcuser=u -rpcpassword=p -rpcclienttimeout=5 echo
[1] 1289444
POST / HTTP/1.1
Host: 127.0.0.1
Connection: close
Content-Length: 55
Content-Type: application/json
Authorization: Basic dTpw

{"method":"echo","params":[],"id":"1","jsonrpc":"2.0"}
error: Authorization failed: Incorrect rpcuser or rpcpassword were specified. Configuration file: (/home/user/.bitcoin/bitcoin.conf)

real    0m0.009s
user    0m0.006s
sys     0m0.007s

hodlinator approved

hodlinator commented at 12:21 PM on October 1, 2026: contributor

ACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3

Disambiguating missing Content-Length from zero to avoid the wait in the latter case makes sense. Verified test by running PR base only with updated test and observing failure. (Also ran the test on the full PR with success).

I played a small part in coming up with the deadline timeout approach used on master but agree on being more lenient as long as the server is sending us a response.

l0rinc commented at 7:01 PM on October 1, 2026: contributor

Concept ACK

achow101 commented at 10:25 PM on October 2, 2026: member

ACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3

DrahtBot requested review from l0rinc on Oct 2, 2026

achow101 merged this on Oct 2, 2026

achow101 closed this on Oct 2, 2026