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
+93 −68
Merged

SafeSocketHandle: avoid potential blocking of finalizer thread #41508

Show file tree
Hide file tree
Changes from 9 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
125 changes: 68 additions & 57 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 @@ -4253,91 +4253,102 @@ protected virtual void Dispose(bool disposing)
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
if (!disposing)
{
int timeout = _closeTimeout;
if (timeout == 0 || !disposing)
{
// Abortive.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle?.CloseAsIs(abortive: true);
}
else
// 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();
tmds marked this conversation as resolved.
Show resolved Hide resolved
}
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
{
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)
int timeout = _closeTimeout;
if (timeout == 0)
{
// Close with existing user-specified linger option.
// Abortive.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");
_handle.CloseAsIs(abortive: false);
_handle?.CloseAsIs(abortive: true);
}
else
{
// 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}");
SocketError errorCode;

if (errorCode != SocketError.Success)
// 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)
{
_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
{
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)
// 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)
{
// 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}");
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.Success || dataAvailable != 0)
if (errorCode != (SocketError)0)
{
// If we have data or don't know, safest thing is to reset.
// We got a timeout - abort.
_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);
// 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
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