-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unix: support SO_REUSEPORT with load balancing for TCP #4407
Conversation
For libuv#386, libuv#570, libuv#1229 Relevant issue: nodejs/node#12228 --------- Signed-off-by: Andy Pan <i@andypan.me>
--------- Signed-off-by: Andy Pan <i@andypan.me>
…ameter --------- Signed-off-by: Andy Pan <i@andypan.me>
--------- Signed-off-by: Andy Pan <i@andypan.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, good work! Ping @libuv/collaborators for some more reviews.
--------- Signed-off-by: Andy Pan <i@andypan.me>
--------- Signed-off-by: Andy Pan <i@andypan.me>
This CI failure on Windows runner seems like a fortuitous thing and it's irrelevant to this PR, right? Could you trigger that failed runner manually? @saghul Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you add a very basic test that checks that uv_tcp_bind()
returns UV_EOPNOTSUPP
on the not supported platforms but succeeds on the others? Thanks
Done! |
--------- Signed-off-by: Andy Pan <i@andypan.me>
Done! |
--------- Signed-off-by: Andy Pan <i@andypan.me>
--------- Signed-off-by: Andy Pan <i@andypan.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits but still LGTM
--------- Signed-off-by: Andy Pan <i@andypan.me>
Is this PR ready to land? Or is there something unresolved? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This missing documentation changes. Can you please add those in https://github.com/libuv/libuv/blob/v1.x/docs/src/tcp.rst? Thanks
Working on it! |
--------- Signed-off-by: Andy Pan <i@andypan.me>
I've made some changes to the docs, PTAL. @santigimeno |
--------- Signed-off-by: Andy Pan <i@andypan.me>
Somehow there are two irrelevant CI failures arising. I assume we need to trigger the GitHub CI manually? |
Triggered them. |
Missed the Windows CI? |
Ups, done! |
Excellent! All CIs have passed, shall we get this PR landed? @saghul @santigimeno |
Nice work Andy! |
Start connecting to the peers after all threads to poll for accepting connections. For libuv#4407 --------- Signed-off-by: Andy Pan <i@andypan.me>
Start connecting to the peers after all threads to poll for accepting connections. Ref: #4407
For #386, #570, #1229
Relevant issue: nodejs/node#12228
The time was not ripe for introducing the
SO_REUSEPORT
TCP in #386 becauseSO_REUSEPORT
s on platforms other than Linux didn't have the capabilities of load balancing, but as time went by, other systems started to extend theSO_REUSEPORT
to distribute workload across all listening sockets: DragonFlyBSD 3.6.0 (2013), FreeBSD 12.0 (2018), Solaris 11.4 (2018), AIX 7.2.5 (2020). #400 was made as a workaround for those who only wanted to enableSO_REUSEPORT
on Linux by creating the socket beforehand, and of course, what it does is beyond theSO_REUSEPORT
. Therefore, I think it's time to reconsider officially supporting this feature.References
@saghul @bnoordhuis