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
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.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
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.
799+ # When port=0, the OS assigns an available ephemeral port. Retrieve
800+ # the actual port so callers know where the server is listening.
801+ actual_port = listener.sockets[0].getsockname()[1]
802+ logger.debug("Listening server on %s:%d should be started" % (addr, actual_port))
803+ cls.listeners[(addr, actual_port)] = listener
804+ port = actual_port
0776 async def create_listen_server(cls, addr, port, callback, proto):
1777 def peer_protocol():
2778 """Returns a function that does the protocol handling for a new
3779 connection. To allow different connections to have different
4780 behaviors, the protocol function is first put in the cls.protos
5781 dict. When the connection is made, the function removes the
6782 protocol function from that dict, and returns it so the event loop
7783 can start executing it."""
8784 response = cls.protos.get((addr, port))
9785 # remove protocol function from dict only when reconnection doesn't need to happen/already happened
10786 if not proto.reconnect:
11787 cls.protos[(addr, port)] = None
12788 return response
13789
14790 if (addr, port) not in cls.listeners:
15791 # When creating a listener on a given (addr, port) we only need to
16792 # do it once. If we want different behaviors for different
17793 # connections, we can accomplish this by providing different
18794 # `proto` functions
19795
20796 listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
21797 # When port=0, the OS assigns an available ephemeral port. Retrieve
22798 # the actual port so callers know where the server is listening.
23799 actual_port = listener.sockets[0].getsockname()[1]
24800 logger.debug("Listening server on %s:%d should be started" % (addr, actual_port))
25801 cls.listeners[(addr, actual_port)] = listener
26802 port = actual_port
27803
28804 cls.protos[(addr, port)] = proto
29805 callback(addr, port)
Is it not the case that inside peer_protocol() and on line 790 port will be 0 which is unintended? That is, those locations expect the actual port?
peer_protocol() is called after port = actual_port and on line 790 port is indeed 0, but that is fine because we are creating a new listener and thus should enter the if. Maybe this would make the intention more obvious:
0- if (addr, port) not in cls.listeners:
1+ if port == 0 or (addr, port) not in cls.listeners:
So, this is good.
ACK 733df32cba96461e5a9d6c8194cedec9366ac755
Just for reference, FreeBSD ephemeral ports’ ranges are configurable. There are two types - for outgoing connections (TCP client) and for accepting incoming connections (TCP server).
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
793@@ -794,8 +794,12 @@ def peer_protocol():
794 # `proto` functions
795
796 listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
797- logger.debug("Listening server on %s:%d should be started" % (addr, port))
nit, feel free to ignore, I think the current diff
0 listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
1- logger.debug("Listening server on %s:%d should be started" % (addr, port))
2- cls.listeners[(addr, port)] = listener
3+ # When port=0, the OS assigns an available ephemeral port. Retrieve
4+ # the actual port so callers know where the server is listening.
5+ actual_port = listener.sockets[0].getsockname()[1]
6+ logger.debug("Listening server on %s:%d should be started" % (addr, actual_port))
7+ cls.listeners[(addr, actual_port)] = listener
8+ port = actual_port
is equivalent to:
0 listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
1+ port = listener.sockets[0].getsockname()[1]
2 logger.debug("Listening server on %s:%d should be started" % (addr, port))
3 cls.listeners[(addr, port)] = listener
Freebsd sets:
0/*
1 * Default local port range, used by IP_PORTRANGE_DEFAULT
2 */
3#define IPPORT_EPHEMERALFIRST 10000
4#define IPPORT_EPHEMERALLAST 65535
So I wonder if this caused https://github.com/bitcoin/bitcoin/issues/34331