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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34186.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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:
# Tor ports are the highest among p2p/rpc/tor, so this should be the first available port.
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.
Sure, makes sense. Though, I still think the specific bug this is referring to in the pull description is a real bug in the code and the comment in the line above the bug is giving the wrong impression that the code would be fine.
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.
Concept ACK
Code Review ACK 733df32cba96461e5a9d6c8194cedec9366ac755
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
776 async def create_listen_server(cls, addr, port, callback, proto):
777 def peer_protocol():
778 """Returns a function that does the protocol handling for a new
779 connection. To allow different connections to have different
780 behaviors, the protocol function is first put in the cls.protos
781 dict. When the connection is made, the function removes the
782 protocol function from that dict, and returns it so the event loop
783 can start executing it."""
784 response = cls.protos.get((addr, port))
785 # remove protocol function from dict only when reconnection doesn't need to happen/already happened
786 if not proto.reconnect:
787 cls.protos[(addr, port)] = None
788 return response
789
790 if (addr, port) not in cls.listeners:
791 # When creating a listener on a given (addr, port) we only need to
792 # do it once. If we want different behaviors for different
793 # connections, we can accomplish this by providing different
794 # `proto` functions
795
796 listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
797 # When port=0, the OS assigns an available ephemeral port. Retrieve
798 # the actual port so callers know where the server is listening.
799 actual_port = listener.sockets[0].getsockname()[1]
800 logger.debug("Listening server on %s:%d should be started" % (addr, actual_port))
801 cls.listeners[(addr, actual_port)] = listener
802 port = actual_port
803
804 cls.protos[(addr, port)] = proto
805 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:
- if (addr, port) not in cls.listeners:
+ if port == 0 or (addr, port) not in cls.listeners:
So, this is good.
Done. Thanks.
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
listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
- logger.debug("Listening server on %s:%d should be started" % (addr, port))
- cls.listeners[(addr, port)] = listener
+ # When port=0, the OS assigns an available ephemeral port. Retrieve
+ # the actual port so callers know where the server is listening.
+ actual_port = listener.sockets[0].getsockname()[1]
+ logger.debug("Listening server on %s:%d should be started" % (addr, actual_port))
+ cls.listeners[(addr, actual_port)] = listener
+ port = actual_port
is equivalent to:
listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
+ port = listener.sockets[0].getsockname()[1]
logger.debug("Listening server on %s:%d should be started" % (addr, port))
cls.listeners[(addr, port)] = listener
Done. Thanks.
ACK ce63d37ebee8d062310a778c5efa6475814188e9
re-ACK ce63d37ebee8d062310a778c5efa6475814188e9
ACK ce63d37ebee8d062310a778c5efa6475814188e9
Freebsd sets:
/*
* Default local port range, used by IP_PORTRANGE_DEFAULT
*/
#define IPPORT_EPHEMERALFIRST 10000
#define IPPORT_EPHEMERALLAST 65535
So I wonder if this caused https://github.com/bitcoin/bitcoin/issues/34331