Skip to content

Commit

Permalink
[release/5.0-rc2] SafeSocketHandle: avoid potential blocking of final…
Browse files Browse the repository at this point in the history 
…izer thread (#41747)

Backport of #41508 to release/5.0-rc2

* SafeSocketHandle: avoid potential blocking of finalizer thread

When the Socket is Disposed, it attempts to make all on-going operations
abort. It does this in a loop, and decides there are no on-going operations
when the reference count becomes zero and the handle gets released.

SafePipeHandle holds a reference to SafeSocketHandle. This prevents the
reference count to drop to zero, and causes the dispose to loop infinitly.

When the Socket is disposed from the finalizer thread, it is no longer used
for operations and we can skip the loop. This avoids blocking the finalizer
thread when the reference count can't drop to zero.

* When disposing from finalizer, fall back to ReleaseHandle

* Add test

* PR feedback

* Fix test

* PR feedback

* Refactor

* Refactor

* Log call to _handle.Dispose

* Handle null handle

Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
  • Loading branch information
@github-actions @tmds
github-actions[bot] and tmds authored Sep 8, 2020
1 parent 96f6793 commit 482494f
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 68 deletions.
145 changes: 77 additions & 68 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4246,99 +4246,108 @@ protected virtual void Dispose(bool disposing)

SetToDisconnected();

// If the safe handle doesn't own the underlying handle, we're done.
SafeSocketHandle handle = _handle;
if (handle != null && !handle.OwnsHandle)
SafeSocketHandle? handle = _handle;
// Avoid side effects when we don't own the handle.
if (handle?.OwnsHandle == true)
{
return;
}

// Close the handle in one of several ways depending on the timeout.
// Ignore ObjectDisposedException just in case the handle somehow gets disposed elsewhere.
try
{
int timeout = _closeTimeout;
if (timeout == 0 || !disposing)
if (!disposing)
{
// Abortive.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle?.CloseAsIs(abortive: true);
// When we are running on the finalizer thread, we don't call CloseAsIs
// because it may lead to blocking the finalizer thread when trying
// to abort on-going operations. We directly dispose the SafeHandle.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.Dispose()");
handle.Dispose();
}
else
{
SocketError errorCode;

// Go to blocking mode.
if (!_willBlock || !_willBlockInternal)
{
bool willBlock;
errorCode = SocketPal.SetBlocking(_handle, false, out willBlock);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{_handle} ioctlsocket(FIONBIO):{errorCode}");
}

if (timeout < 0)
{
// Close with existing user-specified linger option.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle.CloseAsIs(abortive: false);
}
else
// Close the handle in one of several ways depending on the timeout.
// Ignore ObjectDisposedException just in case the handle somehow gets disposed elsewhere.
try
{
// Since our timeout is in ms and linger is in seconds, implement our own sortof linger here.
errorCode = SocketPal.Shutdown(_handle, _isConnected, _isDisconnected, SocketShutdown.Send);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{_handle} shutdown():{errorCode}");

// This should give us a timeout in milliseconds.
errorCode = SocketPal.SetSockOpt(
_handle,
SocketOptionLevel.Socket,
SocketOptionName.ReceiveTimeout,
timeout);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{_handle} setsockopt():{errorCode}");

if (errorCode != SocketError.Success)
int timeout = _closeTimeout;
if (timeout == 0)
{
_handle.CloseAsIs(abortive: true);
// Abortive.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
handle.CloseAsIs(abortive: true);
}
else
{
int unused;
errorCode = SocketPal.Receive(_handle, Array.Empty<byte>(), 0, 0, SocketFlags.None, out unused);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{_handle} recv():{errorCode}");
SocketError errorCode;

// Go to blocking mode.
if (!_willBlock || !_willBlockInternal)
{
bool willBlock;
errorCode = SocketPal.SetBlocking(handle, false, out willBlock);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} ioctlsocket(FIONBIO):{errorCode}");
}

if (errorCode != (SocketError)0)
if (timeout < 0)
{
// We got a timeout - abort.
_handle.CloseAsIs(abortive: true);
// Close with existing user-specified linger option.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
handle.CloseAsIs(abortive: false);
}
else
{
// We got a FIN or data. Use ioctlsocket to find out which.
int dataAvailable = 0;
errorCode = SocketPal.GetAvailable(_handle, out dataAvailable);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{_handle} ioctlsocket(FIONREAD):{errorCode}");

if (errorCode != SocketError.Success || dataAvailable != 0)
// Since our timeout is in ms and linger is in seconds, implement our own sortof linger here.
errorCode = SocketPal.Shutdown(handle, _isConnected, _isDisconnected, SocketShutdown.Send);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} shutdown():{errorCode}");

// This should give us a timeout in milliseconds.
errorCode = SocketPal.SetSockOpt(
handle,
SocketOptionLevel.Socket,
SocketOptionName.ReceiveTimeout,
timeout);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} setsockopt():{errorCode}");

if (errorCode != SocketError.Success)
{
// If we have data or don't know, safest thing is to reset.
_handle.CloseAsIs(abortive: true);
handle.CloseAsIs(abortive: true);
}
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);
int unused;
errorCode = SocketPal.Receive(handle, Array.Empty<byte>(), 0, 0, SocketFlags.None, out unused);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} recv():{errorCode}");

if (errorCode != (SocketError)0)
{
// We got a timeout - abort.
handle.CloseAsIs(abortive: true);
}
else
{
// We got a FIN or data. Use ioctlsocket to find out which.
int dataAvailable = 0;
errorCode = SocketPal.GetAvailable(handle, out dataAvailable);
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} ioctlsocket(FIONREAD):{errorCode}");

if (errorCode != SocketError.Success || dataAvailable != 0)
{
// If we have data or don't know, safest thing is to reset.
handle.CloseAsIs(abortive: true);
}
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);
}
}
}
}
}
}
catch (ObjectDisposedException)
{
NetEventSource.Fail(this, $"handle:{handle}, Closing the handle threw ObjectDisposedException.");
}
}
}
catch (ObjectDisposedException)
{
NetEventSource.Fail(this, $"handle:{_handle}, Closing the handle threw ObjectDisposedException.");
}

// Clean up any cached data
DisposeCachedTaskSocketAsyncEventArgs();
Expand Down
16 changes: 16 additions & 0 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs
View file
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

0 comments on commit 482494f

Please sign in to comment.