Skip to content
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.

Sign up for GitHub

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

Jump to bottom

unix: support SO_REUSEPORT with load balancing for TCP #4407

Merged
merged 12 commits into from
May 21, 2024
Merged

unix: support SO_REUSEPORT with load balancing for TCP #4407

merged 12 commits into from
May 21, 2024

Conversation

panjf2000
Copy link
Contributor

@panjf2000 panjf2000 commented May 17, 2024 •
edited

For #386, #570, #1229

Relevant issue: nodejs/node#12228

The time was not ripe for introducing the SO_REUSEPORT TCP in #386 because SO_REUSEPORTs on platforms other than Linux didn't have the capabilities of load balancing, but as time went by, other systems started to extend the SO_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 enable SO_REUSEPORT on Linux by creating the socket beforehand, and of course, what it does is beyond the SO_REUSEPORT. Therefore, I think it's time to reconsider officially supporting this feature.

References

@saghul @bnoordhuis

@panjf2000
unix: support SO_REUSEPORT with load balancing for TCP
fe5faea 
For libuv#386, libuv#570, libuv#1229

Relevant issue: nodejs/node#12228

---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Update the test case
b2edd24 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Eliminate the compiler warnings of -Wunused-variable and -Wunused-par…
625268c 
…ameter

---------

Signed-off-by: Andy Pan <i@andypan.me>
saghul
saghul reviewed May 17, 2024
View reviewed changes
src/unix/tcp.c Outdated Show resolved Hide resolved
src/unix/tcp.c Outdated Show resolved Hide resolved
test/test-tcp-reuseport.c Show resolved Hide resolved
@panjf2000
Resolve some code review comments
cbe1f8f 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000 panjf2000 requested a review from saghul May 17, 2024 06:52
saghul
saghul approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@saghul saghul left a 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.

@panjf2000
Update the comments of uv__tcp_reuseport()
ccb70b2 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
win: return ERROR_NOT_SUPPORTED for UV_TCP_REUSEPORT
903dde2 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Copy link
Contributor Author

panjf2000 commented May 17, 2024 •
edited

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!

santigimeno
santigimeno approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@santigimeno santigimeno left a 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

@saghul
Copy link
Member

saghul commented May 17, 2024

Could you trigger that failed runner manually?

Done!

@panjf2000
Add a test for unsupported platforms
3302386 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Copy link
Contributor Author

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!

@panjf2000
Consolidate the returned error
a23e1f3 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Fix indents of comments
277fdc8 
---------

Signed-off-by: Andy Pan <i@andypan.me>
santigimeno
santigimeno approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@santigimeno santigimeno left a 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

test/test-tcp-reuseport.c Outdated Show resolved Hide resolved
test/test-tcp-reuseport.c Outdated Show resolved Hide resolved
@panjf2000
Fix the long lines
2a5828b 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Copy link
Contributor Author

Is this PR ready to land? Or is there something unresolved?
@saghul @santigimeno

santigimeno
santigimeno requested changes May 20, 2024
View reviewed changes
Copy link
Member

@santigimeno santigimeno left a 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

@panjf2000
Copy link
Contributor Author

This missing documentation changes. Can you please add those in v1.x/docs/src/tcp.rst? Thanks

Working on it!

@panjf2000
doc: add docs for UV_TCP_REUSEPORT
3cb69dd 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000
Copy link
Contributor Author

panjf2000 commented May 20, 2024 •
edited

This missing documentation changes. Can you please add those in v1.x/docs/src/tcp.rst? Thanks

Working on it!

I've made some changes to the docs, PTAL. @santigimeno

@panjf2000
doc: mention the new flag with versionchanged
220d4d5 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000 panjf2000 requested a review from saghul May 21, 2024 08:01
@panjf2000
Copy link
Contributor Author

Somehow there are two irrelevant CI failures arising. I assume we need to trigger the GitHub CI manually?

@saghul
Copy link
Member

saghul commented May 21, 2024

Somehow there are two irrelevant CI failures arising. I assume we need to trigger the GitHub CI manually?

Triggered them.

@panjf2000
Copy link
Contributor Author

Somehow there are two irrelevant CI failures arising. I assume we need to trigger the GitHub CI manually?

Triggered them.

Missed the Windows CI？

@saghul
Copy link
Member

saghul commented May 21, 2024

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!

@panjf2000
Copy link
Contributor Author

Excellent! All CIs have passed, shall we get this PR landed? @saghul @santigimeno

@saghul saghul merged commit d2d92b7 into libuv:v1.x May 21, 2024
38 checks passed
@saghul
Copy link
Member

saghul commented May 21, 2024

Nice work Andy!

@panjf2000 panjf2000 deleted the tcp-reuseport branch May 21, 2024 11:38
panjf2000 added a commit to panjf2000/libuv that referenced this pull request May 22, 2024
@panjf2000
test: fix the flaky test-tcp-reuseport
9ad0f23 
Start connecting to the peers after all threads
to poll for accepting connections.

For libuv#4407

---------

Signed-off-by: Andy Pan <i@andypan.me>
This was referenced May 22, 2024
Merged
Open
saghul pushed a commit that referenced this pull request May 30, 2024
@panjf2000
test: fix the flaky test-tcp-reuseport
1ee1063 
Start connecting to the peers after all threads
to poll for accepting connections.

Ref: #4407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saghul saghul saghul approved these changes

@santigimeno santigimeno santigimeno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@panjf2000 @saghul @santigimeno