Avoid TcpListener port collisions
#718
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
tnull
force-pushed
the
2025-11-avoid-port-collisions
branch
from
da47569 to
bfda813
Compare
|
Hmm, even with the current changes I didn't get much insight into the |
tnull
force-pushed
the
2025-11-avoid-port-collisions
branch
from
bfda813 to
1b7d919
Compare
|
Now reopened instead of #719. |
|
|
||
| for _ in 0..num_addresses { | ||
| let rand_port = random_port(); | ||
| while listening_addresses.len() < num_addresses { |
There was a problem hiding this comment.
I think the former for loop can remain?
There was a problem hiding this comment.
Well, I'm not sure that there is a guarantee that we don't get the same port back if we drop socket each iteration. I guess we could push them to a Vec to keep them alive while we're still iterating, but this seemed easier.
There was a problem hiding this comment.
Hmm although obvious, I didn't realize that we don't actually use the socket. In that case, it definitely seems better to keep the sockets alive?
There was a problem hiding this comment.
Hmm although obvious, I didn't realize that we don't actually use the socket. In that case, it definitely seems better to keep the sockets alive?
No, why? We def. don't want to keep it alive to allow the Node setup a new one?
There was a problem hiding this comment.
I mean during the iteration, for the reason you brought up?
There was a problem hiding this comment.
Right, but the OS likely gives back random available ports, and we dedup via the HashSet, so we're good?
There was a problem hiding this comment.
Idk if that is always the case for all OSes? Seems there is endless loop potential here. It's just test, fine to leave as is, but it doesn't feel 100% solid.
Previously, we could in tests potentially run into listening port collisions resulting into `InvalidSocketAddress` errors. These errors could surface if we rolled port numbers that either collided with other concurrent tests *or* with other unrelated services running on localhost. Here, we simply let the OS assign us a free port number when setting up the testing nodes, which avoids such collisions altoghether (mod the potential TOCTOU race here, which we ignore for now).
tnull
force-pushed
the
2025-11-avoid-port-collisions
branch
from
1b7d919 to
c0b4933
Compare
|
|
||
| for _ in 0..num_addresses { | ||
| let rand_port = random_port(); | ||
| while listening_addresses.len() < num_addresses { |
There was a problem hiding this comment.
Idk if that is always the case for all OSes? Seems there is endless loop potential here. It's just test, fine to leave as is, but it doesn't feel 100% solid.
3 participants
Second, more thorough attempt:
Previously, we could in tests potentially run into listening port collisions resulting into
InvalidSocketAddresserrors. These errors could surface if we rolled port numbers that either collided with other concurrent tests or with other unrelated services running on localhost.Here, we simply let the OS assign us a free port number when setting up the testing nodes, which avoids such collisions altoghether (mod the potential TOCTOU race here, which we ignore for now).