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

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

2. 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
3. DrahtBot added the label Tests on Mar 17, 2026
4. 
   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 <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

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

    0diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py
    1index 4cd59f8ff1..3a87d126bb 100755
    2--- a/test/functional/p2p_orphan_handling.py
    3+++ b/test/functional/p2p_orphan_handling.py
    4@@ -822,6 +822,7 @@ class OrphanHandlingTest(BitcoinTestFramework):
    5         self.wallet = MiniWallet(self.nodes[0])
    6         self.generate(self.wallet, 160)
    7 
    8+        '''
    9         self.test_arrival_timing_orphan()
   10         self.test_orphan_rejected_parents_exceptions()
   11         self.test_orphan_multiple_parents()
   12@@ -834,6 +835,7 @@ class OrphanHandlingTest(BitcoinTestFramework):
   13         self.test_orphan_handling_prefer_outbound()
   14         self.test_announcers_before_and_after()
   15         self.test_parents_change()
   16+        '''
   17         self.test_maximal_package_protected()
   18 
   19 
   20diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py
   21index a3eb0b0578..5fe6ff0a06 100644
   22--- a/test/functional/test_framework/authproxy.py
   23+++ b/test/functional/test_framework/authproxy.py
   24@@ -167,8 +167,11 @@ class AuthServiceProxy():
   25     def _get_response(self):
   26         req_start_time = time.time()
   27         try:
   28+            if self._service_name == 'testmempoolaccept':
   29+                raise socket.timeout("Fake timeout!")
   30             http_response = self.__conn.getresponse()
   31         except socket.timeout:
   32+            self.__conn.close() # close the connection if the request timed out
   33             raise JSONRPCException({
   34                 'code': -344,
   35                 'message': '%r RPC took longer than %f seconds. Consider '
   

6. DrahtBot added the label CI failed on Mar 17, 2026
7. 
   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.

    02026-03-17T11:57:04.087893Z TestFramework (INFO): Trying to send a transaction when none of Tor or I2P is reachable
    12026-03-17T11:57:04.289600Z TestFramework.socks5 (ERROR): socks5 request handling failed.
    2Traceback (most recent call last):
    3  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 185, in handle
    4    forward_sockets(self.conn, conn_to)
    5    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
    6  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 68, in forward_sockets
    7    data = s.recv(4096)
    8ConnectionResetError: [Errno 54] Connection reset by peer
    92026-03-17T11:57:04.780915Z TestFramework (INFO): Stopping nodes
   102026-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
   112026-03-17T11:57:04.972946Z TestFramework (INFO): Tests successful
   12
   13
   14stderr:
   15/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
   16  self._ready.clear()
   17RuntimeWarning: Enable tracemalloc to get the object allocation traceback
   

8. 
   Bortlesboat commented at 4:16 pm on March 17, 2026: none

   ACK fa9d534bec97a4eac4896f7057b2d1f5a9672c1d — follows the same pattern as the existing OSError handler in _request.
9. 
   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.

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

     02026-03-17T11:57:04.087893Z TestFramework (INFO): Trying to send a transaction when none of Tor or I2P is reachable
     12026-03-17T11:57:04.289600Z TestFramework.socks5 (ERROR): socks5 request handling failed.
     2Traceback (most recent call last):
     3  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 185, in handle
     4    forward_sockets(self.conn, conn_to)
     5    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
     6  File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/socks5.py", line 68, in forward_sockets
     7    data = s.recv(4096)
     8ConnectionResetError: [Errno 54] Connection reset by peer
     92026-03-17T11:57:04.780915Z TestFramework (INFO): Stopping nodes
    102026-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
    112026-03-17T11:57:04.972946Z TestFramework (INFO): Tests successful
    12
    13
    14stderr:
    15/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
    16  self._ready.clear()
    17RuntimeWarning: 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.

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

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

13. 
    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?

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

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

16. 
    rkrux commented at 8:36 am on March 19, 2026: contributor

    Closing PR because others find the current error trace helpful.
17. rkrux closed this on Mar 19, 2026

    ---

Contributors
rkrux DrahtBot Bortlesboat maflcko sedited

Labels
Tests CI failed