[Core] Consolidate find_free_port to network_utils #58304
Conversation
Yicheng-Lu-llll
force-pushed
the
find-free-port-typehint-and-relocation
branch
from
e40b6fc to
ec9c797
Compare
…ls with type hints Signed-off-by: Yicheng-Lu-llll <[email protected]>
Yicheng-Lu-llll
force-pushed
the
find-free-port-typehint-and-relocation
branch
from
ec9c797 to
e1d0e3e
Compare
Yicheng-Lu-llll
requested review from
a team,
edoakes and
jjyao
as code owners
ray-gardener
bot
added
core
python/ray/_common/network_utils.py
Outdated
| """ | ||
| with closing(socket.socket(family, socket.SOCK_STREAM)) as s: | ||
| s.bind(("", 0)) | ||
| s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) |
There was a problem hiding this comment.
what's the purpose of SO_REUSEADDR here? is it to allow subsequent usage of the port to bind to it even if it hasn't been freed by the kernel yet?
There was a problem hiding this comment.
Good catch—this was carried over from the original implementation in python/ray/air/_internal/util.py, and I didn’t re‑evaluate it at the time. I think the intention, as you said, was to allow subsequent processes to bind to the port even if the kernel hadn’t freed it yet, since the purpose of this function is to get a free port.
In reality, a socket that is only bound (not listening or connected) doesn’t enter TIME_WAIT, so the port is freed on close and the next process can bind immediately. Also, SO_REUSEADDR only affects the socket that sets it during its own bind(); it doesn’t prepare the port for others.
I’ll remove this line. Thank you!
There was a problem hiding this comment.
I noticed we also have the following in codebase:
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
s.bind(("", 0))Because SO_REUSEADDR is set, it can return a port that is only bindable with SO_REUSEADDR (e.g., while there are lingering connections in TIME_WAIT). A subsequent bind() (without SO_REUSEADDR) may then fail. So it’s safer to also remove the line here.
There was a problem hiding this comment.
Thanks for the detailed explanation! I agree with the conclusions.
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
|
I've added the Please ping me when this passes |
|
@edoakes Thanks! CI has passed |
This PR is a follow-up to ray-project#56147 (comment) The `find_free_port()` function was duplicated across multiple locations in the codebase. This PR consolidates all implementations into a single canonical location. --------- Signed-off-by: Yicheng-Lu-llll <[email protected]>
This PR is a follow-up to ray-project#56147 (comment) The `find_free_port()` function was duplicated across multiple locations in the codebase. This PR consolidates all implementations into a single canonical location. --------- Signed-off-by: Yicheng-Lu-llll <[email protected]>
This PR is a follow-up to ray-project#56147 (comment) The `find_free_port()` function was duplicated across multiple locations in the codebase. This PR consolidates all implementations into a single canonical location. --------- Signed-off-by: Yicheng-Lu-llll <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
This PR is a follow-up to ray-project#56147 (comment) The `find_free_port()` function was duplicated across multiple locations in the codebase. This PR consolidates all implementations into a single canonical location. --------- Signed-off-by: Yicheng-Lu-llll <[email protected]>
This PR is a follow-up to ray-project#56147 (comment) The `find_free_port()` function was duplicated across multiple locations in the codebase. This PR consolidates all implementations into a single canonical location. --------- Signed-off-by: Yicheng-Lu-llll <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
3 participants
Description
This PR is a follow-up to #56147 (comment)
The
find_free_port()function was duplicated across multiple locations in the codebase. This PR consolidates all implementations into a single canonical location.Related issues
Additional information