Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Unix: fix UDP ReuseAddress with non .NET Core UDP clients #32046

Merged
merged 8 commits into from Oct 9, 2018

Conversation

tmds
Copy link
Member

@tmds tmds commented Aug 31, 2018

Fixes #32027

.NET Core sets SO_REUSEPORT to enable reusing the address with other
sockets. This works when the other sockets also set SO_REUSEPORT.

SO_REUSEADDR can also be used for this. When one application uses
SO_REUSEADDR and another uses SO_REUSEPORT, the second application
will fail to bind.

To implement SocketOptionName.ReuseAddress we need to set both options.
This enables sharing with applications that set SO_REUSEPORT,
SO_REUSEADDR or both.

.NET Core sets SO_REUSEPORT to enable reusing the address with other
sockets. This works when the other sockets also set SO_REUSEPORT.

SO_REUSEADDR can also be used for this. When one application uses
SO_REUSEADDR and another uses SO_REUSEPORT, the second application
will fail to bind.

To implement SocketOptionName.ReuseAddress we need to set both options.
This enables sharing with applications that set SO_REUSEPORT,
SO_REUSEADDR or both.
@tmds
Copy link
Member Author

tmds commented Aug 31, 2018

Fixes https://github.com/dotnet/corefx/issues/32027
This is a regression in 2.1 caused by #24809.

@@ -1842,7 +1840,15 @@ SystemNative_SetSockOpt(intptr_t socket, int32_t socketOptionLevel, int32_t sock
}
}

// An application that sets SO_REUSEPORT/SO_REUSEADDR can share the with another application
Copy link
Member

Choose a reason for hiding this comment

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

share the .. ?

Copy link
Member Author

Choose a reason for hiding this comment

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

"endpoint", pushed a commit to fix this.

@karelz karelz requested a review from a team September 4, 2018 17:06
@davidsh davidsh added area-System.Net.Sockets os-linux Linux OS (any supported distro) and removed area-System.Net labels Sep 4, 2018
@tmds tmds changed the title Unix: fix UDP ReuseAddress with non .NET Core UDP clients [WIP]Unix: fix UDP ReuseAddress with non .NET Core UDP clients Sep 4, 2018
@tmds
Copy link
Member Author

tmds commented Sep 4, 2018

I want to take another look at this. We may want to not set SO_REUSEPORT for UDP on Linux because that will load balance datagrams. We may still want to set it for TCP.
No need to review yet.

@karelz karelz added this to the 3.0 milestone Sep 6, 2018
@tmds
Copy link
Member Author

tmds commented Sep 7, 2018

I looked a bit further into the finer details.

Typical use-cases work as expected: reuse TCP listen port, share UDP multicast.

One case behaves a bit special on Linux: UDP sockets with ReuseAddress set will distribute unicast datagrams accross the different sockets. On Windows/BSD, the last socket that binds 'steals' the port. We can avoid this by not setting SO_REUSEPORT (and only SO_REUSEADDR).

It would be nicest if Linux can stay close to the Windows semantics. On the other hand, the implementation will make the code a bit messier (special case for UDP on Linux). Maybe the behavior is good enough and we want to wait until someone considers&reports this as an issue.

@tmds tmds changed the title [WIP]Unix: fix UDP ReuseAddress with non .NET Core UDP clients Unix: fix UDP ReuseAddress with non .NET Core UDP clients Sep 7, 2018
@stephentoub
Copy link
Member

Maybe the behavior is good enough and we want to wait until someone considers&reports this as an issue.

That sounds fine to me.

@stephentoub
Copy link
Member

Thanks, @tmds. Let's leave it as-is for now to avoid unnecessary churn.

@tmds
Copy link
Member Author

tmds commented Sep 25, 2018

@stephentoub my comment was about adding additional changes to this PR. The PR in its current state has changes which are necessary to fix https://github.com/dotnet/corefx/issues/32027.

@stephentoub stephentoub reopened this Sep 25, 2018
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Can you add a test for this (ideally for all of the modes called out)? From the description of the issue being addressed, it seems like it should be feasible to do. Given the subtleties here, it'll be good to verify that, for example, we get consistent behavior across Linux and macOS.

Otherwise, LGTM.

@stephentoub
Copy link
Member

macOS is unhappy with the new test:

Unhandled Exception of Type System.Net.Sockets.SocketException
Message :
System.Net.Sockets.SocketException : Address already in use
Stack Trace :
   at System.Net.Sockets.Socket.UpdateStatusAfterSocketErrorAndThrowException(SocketError error, String callerName) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 5153
   at System.Net.Sockets.Socket.DoBind(EndPoint endPointSnapshot, SocketAddress socketAddress) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 761
   at System.Net.Sockets.Socket.Bind(EndPoint localEP) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 702
   at System.Net.Sockets.Tests.SocketOptionNameTest.ReuseAddressUdp() in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/tests/FunctionalTests/SocketOptionNameTest.cs:line 523

@tmds
Copy link
Member Author

tmds commented Oct 3, 2018

@dotnet-bot test Linux x64 Release Build please

@tmds
Copy link
Member Author

tmds commented Oct 3, 2018

macOS is unhappy with the new test:

Due to BSD vs Linux differences. I've updated the test.

@@ -490,6 +490,45 @@ public void ExclusiveAddressUseTcp()
}
}

[DllImport("libc", SetLastError = true)]
private unsafe static extern int setsockopt(int socket, int level, int option_name, void* option_value, uint option_len);
Copy link
Member

Choose a reason for hiding this comment

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

Other places we add Unix-specific DllImports like this to tests, we separated them out into their own, e.g., SocketOptionNameTest.Unix.cs partial file. IIRC, it was because of issues they'd otherwise cause on .NET Native when running tests, though I don't remember for sure. Regardless, can you move this one to be consistent?

@stephentoub
Copy link
Member

error CS0103: The name 'setsockopt' does not exist in the current context

@tmds
Copy link
Member Author

tmds commented Oct 4, 2018

error CS0103: The name 'setsockopt' does not exist in the current context

It looks like the SocketOptionNameTest.Unix.cs isn't included in the project. I thought it might be because I had to update the csproj Configurations with separate Unix and Windows, but it still fails when I build it. I don't know what I'm missing.

@tmds
Copy link
Member Author

tmds commented Oct 9, 2018

@dotnet-bot test NETFX x86 Release Build please
@dotnet-bot test Packaging All Configurations x64 Debug Build please
@dotnet-bot test UWP CoreCLR x64 Debug Build please
@dotnet-bot test UWP CoreCLR x64 Release Build please

@tmds
Copy link
Member Author

tmds commented Oct 9, 2018

@dotnet-bot test UWP NETNative x86 Release Build please

@tmds
Copy link
Member Author

tmds commented Oct 9, 2018

CI approves. This should be good to merge.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@stephentoub stephentoub merged commit 0ec9c9d into dotnet:master Oct 9, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…efx#32046)

* Unix: fix UDP ReuseAddress with non .NET Core UDP clients

.NET Core sets SO_REUSEPORT to enable reusing the address with other
sockets. This works when the other sockets also set SO_REUSEPORT.

SO_REUSEADDR can also be used for this. When one application uses
SO_REUSEADDR and another uses SO_REUSEPORT, the second application
will fail to bind.

To implement SocketOptionName.ReuseAddress we need to set both options.
This enables sharing with applications that set SO_REUSEPORT,
SO_REUSEADDR or both.

* Fix comment

* wording: share -> reuse

* Add test

* Fix test on mac

* Move DllImport to separate file

* Add separate csproj Configurations for Windows and Unix

* Add Windows and Unix to Configurations.props


Commit migrated from dotnet/corefx@0ec9c9d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets os-linux Linux OS (any supported distro)
Projects
None yet
8 participants