test: close the HTTPConnection in case of socket timeout #34841

rkrux commented at 11:34 AM on March 17, 2026: contributor

Relates to issue #34731.

In case of a socket timeout in an RPC request, the exception is raised by the authproxy class without closing the http connection. This results in RPCs in the cleanup functions to not be called that results in http.client.CannotSendRequest exceptions.

It can be tested by manually throwing a socket timeout exception from the authproxy class _get_response function. Without the close function call, the http.client.CannotSendRequest exception is thrown in case any RPC is called after the timeout, whereas with the close function call, all the follow-up RPCs can be successfully called.

test: close the HTTPConnection in case of socket timeout

Relates to issue 34731.

In case of a socket timeout in an RPC request, the exception is raised
by the authproxy class without closing the http connection. This results in
RPCs in the cleanup functions to not be called that results in
`http.client.CannotSendRequest` exceptions.

It can be tested by manually throwing a socket timeout exception from the
authproxy class `_get_response` function. Without the `close` function call,
the `http.client.CannotSendRequest` exception is thrown in case any RPC is
called after the timeout, whereas with the close function call, all the
follow-up RPCs can be successfully called.

fa9d534bec

DrahtBot added the label Tests on Mar 17, 2026

DrahtBot commented at 11:34 AM on March 17, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Bortlesboat

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

rkrux commented at 11:36 AM on March 17, 2026: contributor

Following diff can be applied to test the behaviour, and then running the p2p_orphan_handling test. The close function call can be added and removed to check the behaviour in the cleanup function.

diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py
index 4cd59f8ff1..3a87d126bb 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -822,6 +822,7 @@ class OrphanHandlingTest(BitcoinTestFramework):
         self.wallet = MiniWallet(self.nodes[0])
         self.generate(self.wallet, 160)
 
+        '''
         self.test_arrival_timing_orphan()
         self.test_orphan_rejected_parents_exceptions()
         self.test_orphan_multiple_parents()
@@ -834,6 +835,7 @@ class OrphanHandlingTest(BitcoinTestFramework):
         self.test_orphan_handling_prefer_outbound()
         self.test_announcers_before_and_after()
         self.test_parents_change()
+        '''
         self.test_maximal_package_protected()
 
 
diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py
index a3eb0b0578..5fe6ff0a06 100644
--- a/test/functional/test_framework/authproxy.py
+++ b/test/functional/test_framework/authproxy.py
@@ -167,8 +167,11 @@ class AuthServiceProxy():
     def _get_response(self):
         req_start_time = time.time()
         try:
+            if self._service_name == 'testmempoolaccept':
+                raise socket.timeout("Fake timeout!")
             http_response = self.__conn.getresponse()
         except socket.timeout:
+            self.__conn.close() # close the connection if the request timed out
             raise JSONRPCException({
                 'code': -344,
                 'message': '%r RPC took longer than %f seconds. Consider '

DrahtBot added the label CI failed on Mar 17, 2026

rkrux commented at 12:49 PM on March 17, 2026: contributor

I'm not sure if the failing job (CI / macOS native (pull_request)) error is related, I don't get this in my system.

2026-03-17T11:57:04.087893Z TestFramework (INFO): Trying to send a transaction when none of Tor or I2P is reachable
2026-03-17T11:57:04.289600Z TestFramework.socks5 (ERROR): socks5 request handling failed.
Traceback (most recent call last):
  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 185, in handle
    forward_sockets(self.conn, conn_to)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 68, in forward_sockets
    data = s.recv(4096)
ConnectionResetError: [Errno 54] Connection reset by peer
2026-03-17T11:57:04.780915Z TestFramework (INFO): Stopping nodes
2026-03-17T11:57:04.972885Z TestFramework (INFO): Cleaning up /Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/test_runner/test_runner_₿_🏃_20260317_114135/p2p_private_broadcast_74 on exit
2026-03-17T11:57:04.972946Z TestFramework (INFO): Tests successful


stderr:
/opt/homebrew/Cellar/python@3.14/3.14.3_1/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/base_events.py:744: RuntimeWarning: coroutine 'NetworkThread.create_listen_server' was never awaited
  self._ready.clear()
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

Bortlesboat commented at 4:16 PM on March 17, 2026: none

ACK fa9d534bec97a4eac4896f7057b2d1f5a9672c1d — follows the same pattern as the existing OSError handler in _request.

maflcko commented at 10:03 AM on March 18, 2026: member

Seems fine, but my general thinking is that we shouldn't spend too much effort on on trying to clean up after a failure. The tests will (and should) fail either way. Seeing two RPCs fail in the log, or one, is basically irrelevant. In fact, it may be better to fail the next request as well. Otherwise, there is a risk of the test silently continuing over the failure. Also, the debug logs may include additional passing RPCs after the failure/timeout, which may seem confusing.

Again, seems fine to merge this, but I wanted to share a different perspective here.

maflcko commented at 10:07 AM on March 18, 2026: member

I'm not sure if the failing job (CI / macOS native (pull_request)) error is related, I don't get this in my system.

2026-03-17T11:57:04.087893Z TestFramework (INFO): Trying to send a transaction when none of Tor or I2P is reachable
2026-03-17T11:57:04.289600Z TestFramework.socks5 (ERROR): socks5 request handling failed.
Traceback (most recent call last):
  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 185, in handle
    forward_sockets(self.conn, conn_to)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 68, in forward_sockets
    data = s.recv(4096)
ConnectionResetError: [Errno 54] Connection reset by peer
2026-03-17T11:57:04.780915Z TestFramework (INFO): Stopping nodes
2026-03-17T11:57:04.972885Z TestFramework (INFO): Cleaning up /Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/test_runner/test_runner_₿_🏃_20260317_114135/p2p_private_broadcast_74 on exit
2026-03-17T11:57:04.972946Z TestFramework (INFO): Tests successful


stderr:
/opt/homebrew/Cellar/python@3.14/3.14.3_1/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/base_events.py:744: RuntimeWarning: coroutine 'NetworkThread.create_listen_server' was never awaited
  self._ready.clear()
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

Yeah, seems unrelated and intermittent . It could make sense to report it. Also, I wonder if there is an easy diff to reproduce it more often.

rkrux commented at 10:16 AM on March 18, 2026: contributor

Yeah, seems unrelated and intermittent . It could make sense to report it.

Reported #34849.

rkrux commented at 10:29 AM on March 18, 2026: contributor

Seems fine, but my general thinking is that we shouldn't spend too much effort on on trying to clean up after a failure. The tests will (and should) fail either way.

Yes, it did cross my mind that making the cleanup operations work after the first failure is kind of moot.

Seeing two RPCs fail in the log, or one, is basically irrelevant.

The only benefit I see of this PR is to see a clean(er) error trace when the failure is seen the first time. In the #34731 issue, the http.client.CannotSendRequest errror thrown by the generate* RPC (due to the finally block in the cleanup) is a red herring and distracts the developer who is debugging.

maflcko commented at 11:33 AM on March 18, 2026: member

Seeing two RPCs fail in the log, or one, is basically irrelevant.

The only benefit I see of this PR is to see a clean(er) error trace when the failure is seen the first time. In the #34731 issue, the http.client.CannotSendRequest errror thrown by the generate* RPC (due to the finally block in the cleanup) is a red herring and distracts the developer who is debugging.

Yeah, it is not trivial to navigate tracebacks that span several separate exceptions, but I actually found it useful to know that subsequent requests also failed after the timeout on the same connection. Maybe not a surprise, but I guess it can't hurt to know?

rkrux commented at 11:39 AM on March 18, 2026: contributor

but I actually found it useful to know that subsequent requests also failed after the timeout on the same connection

I see, I will close the PR if others also find it useful. I don't feel too strongly about this change but I do prefer a smaller & to-the-point error trace.

sedited commented at 4:30 PM on March 18, 2026: contributor

I see, I will close the PR if others also find it useful. I don't feel too strongly about this change but I do prefer a smaller & to-the-point error trace.

I think I agree with what maflcko said. It is usually a bit confusing to see the other requests at first, but it does help a bit with debugging.

rkrux commented at 8:36 AM on March 19, 2026: contributor

Closing PR because others find the current error trace helpful.

rkrux closed this on Mar 19, 2026