-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Socket: don't perform RST close on Dispose when user called Shutdown #41250
Conversation
src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs
Outdated
Show resolved
Hide resolved
// In case we cancel operations, switch to an abortive close. | ||
// Unless the user requested a normal close using Socket.Shutdown. | ||
bool canceledOperations = false; | ||
bool canAbort = abortive || !_sentShutdown; |
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'm having trouble following what abortive vs canAbort means. Can you clarify?
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.
Before this PR, TryUnblockSocket
always performed an abortive (that is :Connection reset by peer) close.
We're changing to take into account if the user has called Socket.Shutdown
.
canAbort
tells TryUnblockSocket
if it can do an abortive close.
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.
Before this PR, TryUnblockSocket always performed an abortive close.
But TryUnblockSocket was already taking a bool abortive
argument. If it always performed an abortive close, what was the purpose of that argument?
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.
There is a special case for Sockets that don't have the CLOEXEC flag set:
corefx/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs
Lines 379 to 381 in 485441f
// Unless we're doing an abortive close, don't touch sockets which don't have the CLOEXEC flag set. | |
// These may be shared with other processes and we want to avoid disconnecting them. | |
if (!abortive) |
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'm still struggling here, @tmds. Are you saying that both canAbort
and abortive
mean "do an abortive close", but they communicate that information to different parts of the stack? The names are too close to be meaningful.
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 missed this comment.
canAbort
contains whether a CloseAsIs
that was called with abortive: false
, can become a an innerSocket.Close
with abortive: true
.
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'll remove canAbort
and replace it with !hasShutdownSend
.
src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs
Outdated
Show resolved
Hide resolved
@stephentoub Do you have any additional feedback on this? Or are you ready to sign-off? |
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
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.
It might be worth asserting in TryUnblockSocket that canAbort
is always true if abortive
is true. The names are a little confusing. Maybe a tri-state enum would be better, but that's definitely a nit.
There are a number of
There is also a failure of
Is this good to merge? |
I was hoping to continue to discuss #41250 (comment) |
@stephentoub I removed the CI passed except for Azure DevOps unavailable:
|
Thanks, @tmds. |
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
…otnet/corefx#41250) * Socket: don't perform RST close on Dispose when user called Shutdown * Add test * PR feedback * Remove confusing 'canAbort' local * Improve comments Commit migrated from dotnet/corefx@4826532
Fixes https://github.com/dotnet/corefx/issues/41189
Regressed in #38804.
CC @halter73 @davidsh @wfurt @stephentoub @jkotalik