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

Don't try to set SO_REUSEADDR and SO_REUSEPORT on Unix sockets #1625

Merged
merged 15 commits into from May 20, 2024
Merged

Don't try to set SO_REUSEADDR and SO_REUSEPORT on Unix sockets #1625

merged 15 commits into from May 20, 2024

Conversation

panjf2000
Copy link
Contributor

@panjf2000 panjf2000 commented Apr 23, 2024 •
edited

SO_REUSEADDR and SO_REUSEPORT are not supported for TCP and UDP sockets. Setting them on Unix socket via setsockopt somehow doesn't report an error, but it actually won't work.

References

Signed-off-by: Andy Pan i@andypan.me

azat
azat reviewed Apr 24, 2024
View reviewed changes
Copy link
Member

@azat azat 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 test for this? And maybe for https://github.com/libevent/libevent/pull/1616/files as well?

listener.c Show resolved Hide resolved
@panjf2000
Copy link
Contributor Author

LGTM. Can you add a test for this? And maybe for #1616 (files) as well?

Working on it.

@panjf2000
Copy link
Contributor Author

#1582 made some modifications to the test scripts, I need to add tests for this PR on top of that, therefore I think we better merge that PR first so that I can rebase it to avoid some conflicts. @azat

@azat
Copy link
Member

azat commented Apr 29, 2024

#1582 had been merged, you can continue

@panjf2000 panjf2000 force-pushed the reuseport-unix branch 2 times, most recently from 35ecf3e to 9a2d2e9 Compare April 29, 2024 09:52
@panjf2000 panjf2000 requested a review from azat April 29, 2024 09:53
@panjf2000
Copy link
Contributor Author

The CI builds failed automatically because of no runners available, please rerun the CI. @azat

azat
azat reviewed May 4, 2024
View reviewed changes
test/test-reuseport-unix.c Outdated Show resolved Hide resolved
azat
azat reviewed May 4, 2024
View reviewed changes
test/test-reuseport-unix.c Outdated Show resolved Hide resolved
@panjf2000 panjf2000 requested a review from azat May 4, 2024 12:43
azat
azat reviewed May 6, 2024
View reviewed changes
test/regress_listener.c Outdated Show resolved Hide resolved
azat
azat reviewed May 6, 2024
View reviewed changes
test/regress_listener.c Outdated Show resolved Hide resolved
azat
azat reviewed May 6, 2024
View reviewed changes
test/regress_listener.c Outdated Show resolved Hide resolved
azat
azat reviewed May 6, 2024
View reviewed changes
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from minor comments LGTM

@panjf2000 panjf2000 requested a review from azat May 6, 2024 07:46
@azat
Copy link
Member

azat commented May 7, 2024

windows-vs-job (windows-latest, UNOCODE_TEMPORARY_DIRECTORY) fails

@panjf2000
Copy link
Contributor Author

windows-vs-job (windows-latest, UNOCODE_TEMPORARY_DIRECTORY) fails

It seems like there is something to do with the Unicode encoding of the temporary dir path used by this runner, I don't have a quick fix for this in mind, how can I skip this runner? @azat

@panjf2000
Copy link
Contributor Author

This WSAENETDOWN error kept raising when the new test tried to create an AF_UNIX socket on the runner of (windows-latest, UNOCODE_TEMPORARY_DIRECTORY). This error seems to indicates there is something wrong with the machine network:

Network is down.
A socket operation encountered a dead network. This could indicate a serious failure of the network system (that is, the protocol stack that the Windows Sockets DLL runs over), the network interface, or the local network itself.

It's really weird because this runner looks nothing different from other Windows runners but only with the Unicode system temporary path. Do you have any clue about this? @azat

@panjf2000
Skip the test for WSAENETDOWN on Windows with UNOCODE_TEMPORARY_DIREC…
d475cc1 
…TORY

---------

Signed-off-by: Andy Pan <i@andypan.me>

---------

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

I've skipped this test on that specific Windows runner for now, hope we can resolve this suspicious failure later.

@panjf2000
Copy link
Contributor Author

Ping @azat

@azat
Copy link
Member

azat commented May 18, 2024

It's really weird because this runner looks nothing different from other Windows runners but only with the Unicode system temporary path. Do you have any clue about this?

No clue, I'm not a windows expert, and don't want to be this kind of a person. Only Linux.

azat
azat reviewed May 18, 2024
View reviewed changes
test/regress_listener.c Outdated Show resolved Hide resolved
azat
azat reviewed May 18, 2024
View reviewed changes
test/regress_listener.c Outdated Show resolved Hide resolved
@panjf2000
Check if the temporary path is unicode when there is a failure
da26984 
---------

Signed-off-by: Andy Pan <i@andypan.me>
@panjf2000 panjf2000 requested a review from azat May 18, 2024 15:04
@panjf2000
Copy link
Contributor Author

I think this PR is ready to land. @azat

@azat azat merged commit 7a9458c into libevent:master May 20, 2024
58 of 70 checks passed
@panjf2000 panjf2000 deleted the reuseport-unix branch May 20, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@panjf2000 @azat