-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Auto-enable SO_REUSE_UNICASTPORT for ConnectEx #54903
Auto-enable SO_REUSE_UNICASTPORT for ConnectEx #54903
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #48219. Validationhttps://gist.github.com/antonfirsov/ef27d1048dfb362cc19b2c0165dc0d7d The test in the gist succeeds if and only if:
Unfortunately, there is no way to implement an automatic test for this, because of the admin hackery required. /cc @geoffkizer
|
sizeof(int)); | ||
return error == SocketError.Success; | ||
} | ||
catch { } |
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.
I used try-catch
block because SocketProtocolSupportPal
does the same:
runtime/src/libraries/Common/src/System/Net/SocketProtocolSupportPal.Windows.cs
Lines 18 to 38 in f3bad4f
private static bool IsSupported(AddressFamily af) | |
{ | |
Interop.Winsock.EnsureInitialized(); | |
IntPtr INVALID_SOCKET = (IntPtr)(-1); | |
IntPtr socket = INVALID_SOCKET; | |
try | |
{ | |
socket = Interop.Winsock.WSASocketW(af, SocketType.Stream, 0, IntPtr.Zero, 0, (int)Interop.Winsock.SocketConstructorFlags.WSA_FLAG_NO_HANDLE_INHERIT); | |
return | |
socket != INVALID_SOCKET || | |
(SocketError)Marshal.GetLastWin32Error() != SocketError.AddressFamilyNotSupported; | |
} | |
finally | |
{ | |
if (socket != INVALID_SOCKET) | |
{ | |
Interop.Winsock.closesocket(socket); | |
} | |
} | |
} |
Don't know what can potentially throw here.
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.
For native calls like this, usually the only thing that can throw is ObjectDisposedException if the SafeHandle is disposed. Maybe if it's invalid too.
That should never be the case here, but there's no harm in the try/catch just in case.
What OS versions is this supported on? An alternative approach would be to always set the option, but just ignore failure. That would save us from having to explicitly check up front whether it is supported. Actually, now that I think about it... what will happen if the setsockopt call fails? Will that throw and fail the whole Connect? I wonder if we should just ignore failures here always. |
Windows 10+ and Windows 8 with "LDR servicing pack" whatever that means:
I assume |
As long as it doesn't fail the whole connection because some exception gets thrown, I think it's fine. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
The change seems to work, outerloop failures are unrelated, mostly cases of #54955. @geoffkizer can you take another look? |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs
Outdated
Show resolved
Hide resolved
{ | ||
// By enabling SO_REUSE_UNICASTPORT, we defer actual port allocation until the ConnectEx call, | ||
// so it can bind to ports from the Windows auto-reuse port range, if configured by an admin. | ||
// The socket option is supported on Windows 10+, we are ignoring the SocketError in case setsockopt fails. |
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.
Is there any test we could/should write to help validate this?
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.
The problem is, it requires an opt-in registry setting.
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.
What about a test that just validates we've configured the socket as best we can? e.g. that we've set SO_REUSE_UNICASTPORT by default?
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.
Yeah, that seems worthwhile. @antonfirsov can we add this?
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.
Makes sense, will do.
@geoffkizer @stephentoub I attempted to add the test, but looks like What I learned from this experiment is that some of the CI machines actually have the unicast port range configured, so in theory we could test the feature. Unfortunately, the only documented way I was able to find to query these properties is the Looks like we already faced the same issue back in #15888. I deleted the tests for now, and suggest to merge the change as-is. PS: We could maybe test it by checking if |
Seems reasonable. Let's merge this as is. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
CI failures are unrelated, outerloop fails with instances of #54955 most of the time. |
Fixes #48219.
Validation
https://gist.github.com/antonfirsov/ef27d1048dfb362cc19b2c0165dc0d7d
The test in the gist succeeds if and only if:
Set-NetTCPSetting -SettingName InternetCustom -AutoReusePortRangeStartPort 40000 -AutoReusePortRangeNumberOfPorts 2000
) and the PC is rebootedUnfortunately, there is no way to implement an automatic test for this, because of the admin hackery required.
/cc @geoffkizer