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 5 commits
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, finalizing: false);
if (closed)
{
// If the handle was closed just before the call to BindHandle,
Expand Down
34 changes: 16 additions & 18 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 @@ -8,19 +8,6 @@

namespace System.Net.Sockets
{
// This class implements a safe socket handle.
// It uses an inner and outer SafeHandle to do so. The inner
// SafeHandle holds the actual socket, but only ever has one
// reference to it. The outer SafeHandle guards the inner
// SafeHandle with real ref counting. When the outer SafeHandle
// is cleaned up, it releases the inner SafeHandle - since
// its ref is the only ref to the inner SafeHandle, it deterministically
// gets closed at that point - no races with concurrent IO calls.
// This allows Close() on the outer SafeHandle to deterministically
// close the inner SafeHandle, in turn allowing the inner SafeHandle
// to block the user thread in case a graceful close has been
// requested. (It's not legal to block any other thread - such closes
// are always abortive.)
public sealed partial class SafeSocketHandle : SafeHandleMinusOneIsInvalid
{
#if DEBUG
Expand Down Expand Up @@ -84,13 +71,28 @@ protected override bool ReleaseHandle()
return true;
}

internal void CloseAsIs(bool abortive)
internal void CloseAsIs(bool abortive, bool finalizing)
{
#if DEBUG
// If this throws it could be very bad.
try
{
#endif
// 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
// it is no longer used for operations and we can skip TryUnblockSocket fall back to ReleaseHandle.
// This avoids blocking the finalizer thread when TryUnblockSocket is unable to get the reference count to zero.
if (finalizing)
tmds marked this conversation as resolved.
Show resolved Hide resolved
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"finalizing");

Dispose();
return;
}

bool shouldClose = TryOwnClose();

if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"shouldClose={shouldClose}");
Expand All @@ -105,10 +107,6 @@ internal void CloseAsIs(bool abortive)
SpinWait sw = default;
while (!_released)
{
// 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();
}
Expand Down
15 changes: 8 additions & 7 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 @@ -4258,11 +4258,12 @@ protected virtual void Dispose(bool disposing)
try
{
int timeout = _closeTimeout;
if (timeout == 0 || !disposing)
bool finalizing = !disposing;
if (timeout == 0 || finalizing)
{
// Abortive.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle?.CloseAsIs(abortive: true);
_handle?.CloseAsIs(abortive: true, finalizing);
}
else
{
Expand All @@ -4280,7 +4281,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, finalizing);
}
else
{
Expand All @@ -4298,7 +4299,7 @@ protected virtual void Dispose(bool disposing)

if (errorCode != SocketError.Success)
{
_handle.CloseAsIs(abortive: true);
_handle.CloseAsIs(abortive: true, finalizing);
}
else
{
Expand All @@ -4309,7 +4310,7 @@ protected virtual void Dispose(bool disposing)
if (errorCode != (SocketError)0)
{
// We got a timeout - abort.
_handle.CloseAsIs(abortive: true);
_handle.CloseAsIs(abortive: true, finalizing);
}
else
{
Expand All @@ -4321,14 +4322,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, finalizing);
}
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, finalizing);
}
}
}
Expand Down
16 changes: 16 additions & 0 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,22 @@ public async Task NonDisposedSocket_SafeHandlesCollected(bool clientAsync)
});
}

[Fact]
public void SocketWithDanglingReferenceDoesntHangFinalizerThread()
{
CreateSocketWithDanglingReference();
GC.Collect();
GC.WaitForPendingFinalizers();
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void CreateSocketWithDanglingReference()
{
Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
bool dummy = false;
socket.SafeHandle.DangerousAddRef(ref dummy);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static async Task<List<WeakReference>> CreateHandlesAsync(bool clientAsync)
{
Expand Down