test: CI fix: Clean shutdown in Socks5Server

optout21 commented at 9:32 am on March 19, 2026: contributor

Add clean shutdown to Socks5Server test utility, fix a CI failure.

The Socks5Server utility handles multiple incoming connections, which are handled in separate background threads. Its stop() method unblocks and waits for the main background thread cleanly, but it doesn’t attempt to wait for the completion of handler threads. There is no guarantee that the handler threads are finished after stop() returns, which can lead to IO errors.

In the reported sporadic test failures, the test in p2p_private_broadcast.py uses the Socks5 server, and makes a node connect to/through it. Then it stops the Socks5 server, and then it stops the node. However, if a connection handler is still active, that can lead to errors, as the socket is closed.

The change attempts to fix this by storing all handler threads and connections, and attempting to shut them down before stop() returns.

Notes:

I was not able to reliably reproduce the failure locally. The best attempt was to add a 0.25s sleep to Socks5Connection.handle(), right after the self.serv.queue.put(cmdin) line.
With this change, in the p2p_private_broadcast.py test there are some handler threads that can’t be joined cleanly, even with the forced close of sockets, the join times out (after 2s). As a result, the test execution time has increased by several seconds.

DrahtBot added the label Tests on Mar 19, 2026

DrahtBot commented at 9:33 am on March 19, 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. A summary of reviews will appear here.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

# Forwarding socket, stored for close on stop -> # Forwarding socket, stored for closing on stop [“close” is used as a noun; “closing” better matches the intended meaning]
# Set to False when stop initiated -> # Set to False when stop is initiated [Missing “is” makes the sentence slightly unclear]
# Wrapper for thread.join(), which may throw for daemonic threads (in late stages of finalization) -> # Wrapper for thread.join(), which may throw for daemon threads (in late stages of finalization) [“daemonic” is likely a misspelling/incorrect term vs the common “daemon”; may confuse readers]
# if there are active handler, close them -> # if there are active handlers, close them [Plural mismatch: “handler” vs “them”]

2026-03-25 13:50:43

optout21 renamed this:
~~test: Clean shutdown in Socks5Server (CI failure)~~
test: Clean shutdown in Socks5Server (CI fix)
on Mar 19, 2026

optout21 closed this on Mar 20, 2026

optout21 reopened this on Mar 20, 2026

fanquake renamed this:
~~test: Clean shutdown in Socks5Server (CI fix)~~
test: Clean shutdown in Socks5Server
on Mar 20, 2026

fanquake commented at 4:43 am on March 23, 2026: member

cc @vasild

optout21 renamed this:
~~test: Clean shutdown in Socks5Server~~
test: CI fix: Clean shutdown in Socks5Server
on Mar 23, 2026

in test/functional/test_framework/socks5.py:113 in 355f175c13 outdated

112@@ -113,6 +113,8 @@ class Socks5Connection():
113     def __init__(self, serv, conn):

vasild commented at 3:19 pm on March 24, 2026:

nit: might prefix the commit message’s first line with test:

optout21 commented at 1:50 pm on March 25, 2026:

done

in test/functional/test_framework/socks5.py:274 in 355f175c13

265@@ -240,3 +266,14 @@ def stop(self):
266         s.connect(self.conf.addr)
267         s.close()
268         self.thread.join()
269+        # if there are active handler, close them
270+        with self._handlers_lock:
271+            items = list(self.handlers)
272+        for thread, conn in items:
273+            # check if thread is still active
274+            thread.join(timeout=0)

vasild commented at 3:36 pm on March 24, 2026:

https://docs.python.org/3/library/threading.html#meth-thread-join

If an attempt is made to join a running daemonic thread in late stages of Python finalization join() raises a PythonFinalizationError.

Our threads are “demonic” because we do thread.daemon = True when creating them.

optout21 commented at 1:33 pm on March 25, 2026:

Good point. Since the exception only may occur in late stages of finalization, I think the correct handling is to wrap into a try, and ignore the exception. I do this in a helper method now.

in test/functional/test_framework/socks5.py:209 in 355f175c13 outdated

205                 logger.debug("Keeping client connection alive")
206 
207+    def force_close(self):
208+        # close underlying connections
209+        try:
210+            self.conn.close()

vasild commented at 3:39 pm on March 24, 2026:

The new method force_close() will be enough to cancel the loop in forward_sockets() because it closes the sockets, but I think it may end up raise IOError('Exceptional condition on socket'), not a graceful exit.

optout21 commented at 1:36 pm on March 25, 2026:

The same exception can occur without force_close(), and it is handled in an except in handle(). If the proxy is not stopped (or stopped ‘half-way’ with the current stop()), and the connection initiator or receiver closes the connection (e.g. as the node is stopped), the same exception occurs (socked closed while in a blocking read), and it is handled. So using the force_close() triggers the same exception, but it does so before stop() returns.

in test/functional/test_framework/socks5.py:186 in 355f175c13

183@@ -182,6 +184,7 @@ def handle(self):
184                     logger.debug(f"Serving connection to {requested_to}, will redirect it to "
185                                  f"{dest['actual_to_addr']}:{dest['actual_to_port']} instead")
186                     with socket.create_connection((dest["actual_to_addr"], dest["actual_to_port"])) as conn_to:

vasild commented at 3:42 pm on March 24, 2026:

comment in arbitrary location

I think it would be better to set self.running to False (already done in stop()) and check for that in forward_sockets() and when it becomes False then exist gracefully from forward_sockets(). Then in stop() wait indefinitely for handler threads to exit by themselves (they will since they will be checking self.serv.running. Maybe that running flag would need a mutex.

optout21 commented at 1:47 pm on March 25, 2026:

forward_sockets() does a blocking select & read on the sockets, in a loop. If the sockets exist, but not yet closed nor written to, it will block there. While blocking, it cannot check for the running flag. If we want to make sure that when stop exists, the handling threads exit, we need to unblock them. Just waiting could cause a deadlock. Another way would be to have a special socket, and include that in the select call, and use that to unblock, to give a chance for checking the stop flag. The same is achieved by closing the sockets, which may cause IOException, but I don’t think that’s a problem, as described earlier. I’m open to using a special signaling socket, if you prefer.

I did add an extra check for running before calling forward_sockets().

As for the mutex, I added it (though I think setting/gettin a single bool value is Python is fine).

optout21 commented at 12:48 pm on March 25, 2026: contributor

Update: I’ve managed to force-reproduce the issue, and the present fix appears to indeed fix the problem. For details, see: #34849 (comment) @vasild , thanks for the review, I will address them soon.

test: Add clean shutdown to Socks5Server

The `Socks5Server` utility handles multiple incoming connections,
which are handled in separate background threads.
The `stop()` method unblocks and waits for the main background thread
cleanly, but it doesn't attempt to wait for any handler threads.
This change stores handler threads and connections, and attempts
to shut them down before `stop()` returns.

bf7fc3d865

optout21 force-pushed on Mar 25, 2026

optout21 commented at 10:04 pm on March 25, 2026: contributor

Applied changed following review comments:

Guard against potential exception in thread.join(), that can occur with daemonic threads
Add an extra check on running before calling forward_sockets()
Protect the running flag by a lock

test: CI fix: Clean shutdown in Socks5Server #34863

Reviews

LLM Linter (✨ experimental)