Use port=0 for dynamic port allocation in test framework components to avoid intermittent “address already in use” errors when running tests concurrently or when ports are stuck in TIME_WAIT state. Example: #29415 (review)
Changes:
socks5.py and p2p.py to support dynamic port allocationfeature_proxy.py and feature_anchors.py to use port=0
Use port=0 for dynamic port allocation in test framework components
to avoid "address already in use" errors from concurrent tests or
ports stuck in TIME_WAIT state from previous test runs.
Changes:
- socks5.py: Update conf.addr after bind() to reflect actual port
- p2p.py: Retrieve actual port after create_server() when port=0
- feature_proxy.py: Use port=0 for all SOCKS5 proxy servers
- feature_anchors.py: Use port=0 for onion proxy server
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34186.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
ACK 733df32
Tested on macOS - both feature_proxy.py and feature_anchors.py pass, including when run at the same time.
Use
port=0for dynamic port allocation in test framework components to avoid intermittent “address already in use” errors when running tests concurrently or when ports are stuck in TIME_WAIT state. Example: #29415 (comment)
I don’t think this change here avoids this class of error. For example, the rpc ports are still assigned in the old fashion, so this may still happen. Also, the code comment you are referring to is wrong:
0 # Tor ports are the highest among p2p/rpc/tor, so this should be the first available port.
1 ports_base = tor_port(MAX_NODES) + 1
What the code does is that it picks the first tor port of the next test to run.
Generally, calling one of the *_port helpers to get a port number, and then manually modifying it is confusing and brittle and should not be done. See also #33260, which tried to clean up this bad pattern.
Concept ACK
Make sense to me to let the OS deal with the port assignments instead of the makeshift test_runner.py orchestration whenever possible (which is the case for the socks5 proxy of the python framework). This should help with the planned extensions of using socks5 to imitate automatic outbound peers, behind which there isn’t necessarily a full node with an rpc port, it could also be a python p2p, so it should also solve the issue in #33954 (review) - I will try that out.
I don’t think this change here avoids this class of error. For example, the rpc ports are still assigned in the old fashion, so this may still happen.
This is not the scope of this PR, since also the p2p_port() calls in test_runner.py are not changed to dynamic port allocation here - this change only affects the socks5 proxy. But even if it remains the case that running functional tests in parallel (unless it’s done under a single test_runner.py command) can lead to intermittent failures due to port collisions, this seem like a clear improvement.
I left a comment here with what I think is the correct fix: #29415 (review)
I don’t think this change here avoids this class of error. For example, the rpc ports are still assigned in the old fashion, so this may still happen.
This is not the scope of this PR, since also the
p2p_port()calls intest_runner.pyare not changed to dynamic port allocation here - this change only affects the socks5 proxy. But even if it remains the case that running functional tests in parallel (unless it’s done under a singletest_runner.pycommand) can lead to intermittent failures due to port collisions, this seem like a clear improvement.
Maybe I am missing something obvious, but if parts of the tests use dynamic port assignment, and other parts of the tests use “deterministic” port assignment, then how will this not lead to more issues of the kind? For example, there could be a test which dynamically assigns port X, but the very next test to be started will call one of the *_port() functions, which happens to return this port as well. The test will fail, no?
Maybe I am missing something obvious, but if parts of the tests use dynamic port assignment, and other parts of the tests use “deterministic” port assignment, then how will this not lead to more issues of the kind?
By making sure that the ranges don’t overlap: The *_port() functions with their formulas use ports from 11000 - 26000 by default, whereas ephemeral ports start at 32768 on Linux systems (49152 on some others systems). This means we don’t have too much ports left for additional *_port() functions that could be added in the future. Note that the functional tests already make use of ephemeral ports for the python p2ps, so if there were collisions we should be seeing intermittent failures today irrespective of the changes discussed here.
Ah right. I wonder if it could make sense to check for that early, and abort if a violation has been found in the test logic:
0diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
1index 46e5435351..9936bb8aee 100644
2--- a/test/functional/test_framework/util.py
3+++ b/test/functional/test_framework/util.py
4@@ -482,6 +482,7 @@ def get_rpc_proxy(url: str, node_number: int, *, timeout: Optional[int]=None, co
5
6 def p2p_port(n):
7 assert n <= MAX_NODES
8+ assert PORT_MIN + PORT_RANGE * 2 < 32768
9 return PORT_MIN + n + (MAX_NODES * PortSeed.n) % (PORT_RANGE - 1 - MAX_NODES)
10
11
