-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Socket: don't disconnect Socket for unknown/unsupported socket options. #59925
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #59055. @antonfirsov @dotnet/ncl ptal.
|
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.
Thanks! I have only cosmetic concerns, otherwise looks good.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketOptionNameTest.cs
Outdated
Show resolved
Hide resolved
@antonfirsov can you take another look? I've addressed your feedback. I've also made some additional changes. These have to do with us being able to detect use of an unknown option name (as We no longer disconnect for |
Or alternatively, we could never disconnect? |
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 current changes look good.
We no longer disconnect for
ENOPROTOOPT
, but we keep disconnecting for the other errors (so also for unknown levels).
I see you reverted the attempt to fix this. Shall we merge this PR as-is, and maybe open a new issue to track/discuss possible enhancements?
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks @antonfirsov, this is good to merge. The alternative is to never disconnect for socket option errors. We can consider it if someone asks for it. We don't need to create an issue now. |
Fixes #59055.
@antonfirsov @dotnet/ncl ptal.