From 46ee6d59883aa8d8ca900b001e564a0e075c4e3e Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 28 Aug 2020 10:15:40 +0200 Subject: [PATCH 01/10] 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. --- .../Net/Sockets/SafeSocketHandle.Windows.cs | 2 +- .../System/Net/Sockets/SafeSocketHandle.cs | 27 ++++++++++++------- .../src/System/Net/Sockets/Socket.cs | 12 ++++----- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs index 29b3f7da3ea4e5..01457e05d6758f 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs @@ -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); if (closed) { // If the handle was closed just before the call to BindHandle, diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs index 5293c4728bc2b7..86beed98d20989 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs @@ -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. @@ -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. + // 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); diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index 9fbc3c4c9a28b6..fe3fe25a74a39e 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -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 { @@ -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 { @@ -4298,7 +4298,7 @@ protected virtual void Dispose(bool disposing) if (errorCode != SocketError.Success) { - _handle.CloseAsIs(abortive: true); + _handle.CloseAsIs(abortive: true, disposing); } else { @@ -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 { @@ -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); } } } From d83eb5ced457a51b0dc1834df8154ec38f149bd4 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 28 Aug 2020 13:52:36 +0200 Subject: [PATCH 02/10] When disposing from finalizer, fall back to ReleaseHandle --- .../System/Net/Sockets/SafeSocketHandle.cs | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs index 86beed98d20989..186f3d975fa50d 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs @@ -91,6 +91,22 @@ internal void CloseAsIs(bool abortive, bool disposing) 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 (disposing=false) + // it is 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 (!disposing) + { + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"dispose"); + + Debug.Assert(abortive); + Dispose(); + return; + } + bool shouldClose = TryOwnClose(); if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"shouldClose={shouldClose}"); @@ -101,23 +117,12 @@ internal void CloseAsIs(bool abortive, bool disposing) { bool canceledOperations = false; - // 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. - // This avoids blocking the finalizer thread when TryUnblockSocket is unable to get the reference count to zero. - Debug.Assert(disposing || _released); - if (disposing) + // Wait until it's safe. + SpinWait sw = default; + while (!_released) { - // Wait until it's safe. - SpinWait sw = default; - while (!_released) - { - canceledOperations |= TryUnblockSocket(abortive); - sw.SpinOnce(); - } + canceledOperations |= TryUnblockSocket(abortive); + sw.SpinOnce(); } CloseHandle(abortive, canceledOperations); From f42f98454ecd79112a9e45567d0044e2fdf0d408 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 28 Aug 2020 13:52:43 +0200 Subject: [PATCH 03/10] Add test --- .../tests/FunctionalTests/DisposedSocketTests.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs index a19a497ca61b3a..acb2de1beb0100 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs @@ -761,6 +761,16 @@ public async Task NonDisposedSocket_SafeHandlesCollected(bool clientAsync) }); } + [Fact] + public void SocketWithDanglingReferenceDoesntHangFinalizerThread() + { + Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp); + bool dummy = false; + socket.SafeHandle.DangerousAddRef(ref dummy); + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + [MethodImpl(MethodImplOptions.NoInlining)] private static async Task> CreateHandlesAsync(bool clientAsync) { From 652ac74fa75baed1f8a0038a80aa4f41660874a5 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 28 Aug 2020 14:03:08 +0200 Subject: [PATCH 04/10] PR feedback --- .../Net/Sockets/SafeSocketHandle.Windows.cs | 2 +- .../System/Net/Sockets/SafeSocketHandle.cs | 24 ++++--------------- .../src/System/Net/Sockets/Socket.cs | 15 ++++++------ 3 files changed, 14 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs index 01457e05d6758f..2d162fbeb24e95 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs @@ -57,7 +57,7 @@ internal ThreadPoolBoundHandle GetOrAllocateThreadPoolBoundHandle(bool trySkipCo { bool closed = IsClosed; bool alreadyBound = !IsInvalid && !IsClosed && (exception is ArgumentException); - CloseAsIs(abortive: false, disposing: true); + CloseAsIs(abortive: false, finalizing: false); if (closed) { // If the handle was closed just before the call to BindHandle, diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs index 186f3d975fa50d..828a1e39325de8 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs @@ -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 @@ -84,7 +71,7 @@ protected override bool ReleaseHandle() return true; } - internal void CloseAsIs(bool abortive, bool disposing) + internal void CloseAsIs(bool abortive, bool finalizing) { #if DEBUG // If this throws it could be very bad. @@ -95,14 +82,13 @@ internal void CloseAsIs(bool abortive, bool disposing) // 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 fall back to ReleaseHandle. + // 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 (!disposing) + if (finalizing) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"dispose"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"finalizing"); - Debug.Assert(abortive); Dispose(); return; } diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index fe3fe25a74a39e..8d4fc885a26583 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -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, disposing); + _handle?.CloseAsIs(abortive: true, finalizing); } else { @@ -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, disposing); + _handle.CloseAsIs(abortive: false, finalizing); } else { @@ -4298,7 +4299,7 @@ protected virtual void Dispose(bool disposing) if (errorCode != SocketError.Success) { - _handle.CloseAsIs(abortive: true, disposing); + _handle.CloseAsIs(abortive: true, finalizing); } else { @@ -4309,7 +4310,7 @@ protected virtual void Dispose(bool disposing) if (errorCode != (SocketError)0) { // We got a timeout - abort. - _handle.CloseAsIs(abortive: true, disposing); + _handle.CloseAsIs(abortive: true, finalizing); } else { @@ -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, disposing); + _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, disposing); + _handle.CloseAsIs(abortive: false, finalizing); } } } From a69ccb446a681fce0c3c644e3510a1e888b02c7d Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 28 Aug 2020 15:24:25 +0200 Subject: [PATCH 05/10] Fix test --- .../tests/FunctionalTests/DisposedSocketTests.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs index acb2de1beb0100..5e670126c9d8cd 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs @@ -763,12 +763,18 @@ 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); - GC.Collect(); - GC.WaitForPendingFinalizers(); } [MethodImpl(MethodImplOptions.NoInlining)] From 930821bc3a09a4b120fa84a31c4dd68600b74345 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 31 Aug 2020 08:53:20 +0200 Subject: [PATCH 06/10] PR feedback --- .../Net/Sockets/SafeSocketHandle.Windows.cs | 2 +- .../System/Net/Sockets/SafeSocketHandle.cs | 34 ++++++++++--------- .../src/System/Net/Sockets/Socket.cs | 21 +++++++----- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs index 2d162fbeb24e95..29b3f7da3ea4e5 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs @@ -57,7 +57,7 @@ internal ThreadPoolBoundHandle GetOrAllocateThreadPoolBoundHandle(bool trySkipCo { bool closed = IsClosed; bool alreadyBound = !IsInvalid && !IsClosed && (exception is ArgumentException); - CloseAsIs(abortive: false, finalizing: false); + CloseAsIs(abortive: false); if (closed) { // If the handle was closed just before the call to BindHandle, diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs index 828a1e39325de8..5293c4728bc2b7 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs @@ -8,6 +8,19 @@ 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 @@ -71,28 +84,13 @@ protected override bool ReleaseHandle() return true; } - internal void CloseAsIs(bool abortive, bool finalizing) + internal void CloseAsIs(bool abortive) { #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) - { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"finalizing"); - - Dispose(); - return; - } - bool shouldClose = TryOwnClose(); if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"shouldClose={shouldClose}"); @@ -107,6 +105,10 @@ internal void CloseAsIs(bool abortive, bool finalizing) 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(); } diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index 8d4fc885a26583..48998194dfd58c 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -4258,12 +4258,11 @@ protected virtual void Dispose(bool disposing) try { int timeout = _closeTimeout; - bool finalizing = !disposing; - if (timeout == 0 || finalizing) + if (timeout == 0 || !disposing) { // Abortive. if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()"); - _handle?.CloseAsIs(abortive: true, finalizing); + _handle?.CloseAsIs(abortive: true); } else { @@ -4281,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, finalizing); + _handle.CloseAsIs(abortive: false); } else { @@ -4299,7 +4298,7 @@ protected virtual void Dispose(bool disposing) if (errorCode != SocketError.Success) { - _handle.CloseAsIs(abortive: true, finalizing); + _handle.CloseAsIs(abortive: true); } else { @@ -4310,7 +4309,7 @@ protected virtual void Dispose(bool disposing) if (errorCode != (SocketError)0) { // We got a timeout - abort. - _handle.CloseAsIs(abortive: true, finalizing); + _handle.CloseAsIs(abortive: true); } else { @@ -4322,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, finalizing); + _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, finalizing); + _handle.CloseAsIs(abortive: false); } } } @@ -4354,7 +4353,11 @@ public void Dispose() ~Socket() { - Dispose(false); + // We don't call Dispose because it may lead to blocking the finalizer thread + // when SocketSafeHandle.CloseAsIs tries to abort on-going operations. + // Because we are running on the finalizer thread, there are no ongoing operations to be aborted + // and we can directly dispose the SafeHandle. + _handle?.Dispose(); } // This version does not throw. From 4a5251216b90e0b18380757cec66a72df88e4f85 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 31 Aug 2020 08:57:22 +0200 Subject: [PATCH 07/10] Refactor --- .../src/System/Net/Sockets/Socket.cs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index 48998194dfd58c..42bd54412c8f71 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -4253,12 +4253,21 @@ protected virtual void Dispose(bool disposing) return; } + // 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 (!disposing) + { + _handle.Dispose(); + 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 (timeout == 0) { // Abortive. if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()"); @@ -4353,11 +4362,7 @@ public void Dispose() ~Socket() { - // We don't call Dispose because it may lead to blocking the finalizer thread - // when SocketSafeHandle.CloseAsIs tries to abort on-going operations. - // Because we are running on the finalizer thread, there are no ongoing operations to be aborted - // and we can directly dispose the SafeHandle. - _handle?.Dispose(); + Dispose(false); } // This version does not throw. From ac133c2cc729c561a0791168f19fe3907045b640 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 31 Aug 2020 09:04:47 +0200 Subject: [PATCH 08/10] Refactor --- .../src/System/Net/Sockets/Socket.cs | 125 +++++++++--------- 1 file changed, 63 insertions(+), 62 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index 42bd54412c8f71..a48440c46f4e49 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -4253,100 +4253,101 @@ protected virtual void Dispose(bool disposing) return; } - // 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 (!disposing) { + // 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. _handle.Dispose(); - 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 + else { - int timeout = _closeTimeout; - if (timeout == 0) - { - // Abortive. - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()"); - _handle?.CloseAsIs(abortive: true); - } - 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(), 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(), 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 From 44861ce9fe247b6420fc7570dbf43fbd245d4de6 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 31 Aug 2020 09:08:15 +0200 Subject: [PATCH 09/10] Log call to _handle.Dispose --- .../System.Net.Sockets/src/System/Net/Sockets/Socket.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index a48440c46f4e49..b6e7158c9bcf1b 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -4258,6 +4258,7 @@ protected virtual void Dispose(bool disposing) // 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 From 2abef469f0cc60e756bc2377c61cf9571cb295be Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 31 Aug 2020 11:42:57 +0200 Subject: [PATCH 10/10] Handle null handle --- .../src/System/Net/Sockets/Socket.cs | 150 +++++++++--------- 1 file changed, 74 insertions(+), 76 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index b6e7158c9bcf1b..05f90292ff9db7 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -4246,108 +4246,106 @@ 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; - } - - if (!disposing) - { - // 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 - { - // 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) - { - // 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(); + } + 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) + int timeout = _closeTimeout; + if (timeout == 0) { - 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. + // 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}"); - - if (errorCode != SocketError.Success) + 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) { - _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(), 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); + 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(), 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. - _handle.CloseAsIs(abortive: true); + // 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."); + } } }