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

SafeSocketHandle: avoid potential blocking of finalizer thread #41508

Merged
merged 10 commits into from
Sep 1, 2020
Merged

SafeSocketHandle: avoid potential blocking of finalizer thread #41508

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6d85a49
SafeSocketHandle: avoid potential blocking of finalizer thread
tmds Aug 28, 2020
ccc41c9
When disposing from finalizer, fall back to ReleaseHandle
tmds Aug 28, 2020
8c41150
Add test
tmds Aug 28, 2020
43874bb
PR feedback
tmds Aug 28, 2020
99de1e5
Fix test
tmds Aug 28, 2020
16e62fa
PR feedback
tmds Aug 31, 2020
b88432f
Refactor
tmds Aug 31, 2020
c52ab27
Refactor
tmds Aug 31, 2020
ec7b055
Log call to _handle.Dispose
tmds Aug 31, 2020
a44fe82
Handle null handle
tmds Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ internal ThreadPoolBoundHandle GetOrAllocateThreadPoolBoundHandle(bool trySkipCo
{
bool closed = IsClosed;
bool alreadyBound = !IsInvalid && !IsClosed && (exception is ArgumentException);
CloseAsIs(abortive: false);
CloseAsIs(abortive: false, disposing: true);
tmds marked this conversation as resolved.
Show resolved Hide resolved
if (closed)
{
// If the handle was closed just before the call to BindHandle,
Expand Down
27 changes: 17 additions & 10 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ protected override bool ReleaseHandle()
return true;
}

internal void CloseAsIs(bool abortive)
internal void CloseAsIs(bool abortive, bool disposing)
{
#if DEBUG
// If this throws it could be very bad.
Expand All @@ -101,16 +101,23 @@ internal void CloseAsIs(bool abortive)
{
bool canceledOperations = false;

// Wait until it's safe.
SpinWait sw = default;
while (!_released)
// When the handle was not released due it being used, we try to make those on-going calls return.
// TryUnblockSocket will unblock current operations but it doesn't prevent
// a new one from starting. So we must call TryUnblockSocket multiple times.
//
// When the Socket is disposed from the finalizer thread (disposing=false)
// it is longer used for operations and we can skip TryUnblockSocket.
tmds marked this conversation as resolved.
Show resolved Hide resolved
// This avoids blocking the finalizer thread when TryUnblockSocket is unable to get the reference count to zero.
Debug.Assert(disposing || _released);
if (disposing)
{
// The socket was not released due to the SafeHandle being used.
// Try to make those on-going calls return.
// On Linux, TryUnblockSocket will unblock current operations but it doesn't prevent
// a new one from starting. So we must call TryUnblockSocket multiple times.
canceledOperations |= TryUnblockSocket(abortive);
sw.SpinOnce();
// Wait until it's safe.
SpinWait sw = default;
while (!_released)
{
canceledOperations |= TryUnblockSocket(abortive);
sw.SpinOnce();
}
}

CloseHandle(abortive, canceledOperations);
Expand Down
12 changes: 6 additions & 6 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4262,7 +4262,7 @@ protected virtual void Dispose(bool disposing)
{
// Abortive.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle?.CloseAsIs(abortive: true);
_handle?.CloseAsIs(abortive: true, disposing);
}
else
{
Expand All @@ -4280,7 +4280,7 @@ protected virtual void Dispose(bool disposing)
{
// Close with existing user-specified linger option.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle.CloseAsIs(abortive: false);
_handle.CloseAsIs(abortive: false, disposing);
}
else
{
Expand All @@ -4298,7 +4298,7 @@ protected virtual void Dispose(bool disposing)

if (errorCode != SocketError.Success)
{
_handle.CloseAsIs(abortive: true);
_handle.CloseAsIs(abortive: true, disposing);
}
else
{
Expand All @@ -4309,7 +4309,7 @@ protected virtual void Dispose(bool disposing)
if (errorCode != (SocketError)0)
{
// We got a timeout - abort.
_handle.CloseAsIs(abortive: true);
_handle.CloseAsIs(abortive: true, disposing);
}
else
{
Expand All @@ -4321,14 +4321,14 @@ protected virtual void Dispose(bool disposing)
if (errorCode != SocketError.Success || dataAvailable != 0)
{
// If we have data or don't know, safest thing is to reset.
_handle.CloseAsIs(abortive: true);
_handle.CloseAsIs(abortive: true, disposing);
}
else
{
// We got a FIN. It'd be nice to block for the remainder of the timeout for the handshake to finish.
// Since there's no real way to do that, close the socket with the user's preferences. This lets
// the user decide how best to handle this case via the linger options.
_handle.CloseAsIs(abortive: false);
_handle.CloseAsIs(abortive: false, disposing);
}
}
}
Expand Down