From 6d85a49955425a4e20aa4f5659f7a9e91eab5eb6 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 29b3f7da3ea4e..01457e05d6758 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 5293c4728bc2b..86beed98d2098 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 9fbc3c4c9a28b..fe3fe25a74a39 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 ccc41c9fed881d5ab25170e69a7153a95f2d8b68 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 86beed98d2098..186f3d975fa50 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 8c41150d130d002b0ac7230a5307c4ba9ba7482c 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 a19a497ca61b3..acb2de1beb010 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 43874bbddfc4591cbe9f38d14c3b5f3f7f1fb6ae 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 01457e05d6758..2d162fbeb24e9 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 186f3d975fa50..828a1e39325de 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 fe3fe25a74a39..8d4fc885a2658 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 99de1e558a60a416c791e36e2c66620c17d77809 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 acb2de1beb010..5e670126c9d8c 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 16e62fa67ce9eacdd6e14d74671d2a73083c9e5a 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 2d162fbeb24e9..29b3f7da3ea4e 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 828a1e39325de..5293c4728bc2b 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 8d4fc885a2658..48998194dfd58 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 b88432fe5c7792c5245448d2ed8f0dfb795c8ff8 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 48998194dfd58..42bd54412c8f7 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 c52ab27d454349b95bf09c383544977e396563c8 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 42bd54412c8f7..a48440c46f4e4 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 ec7b0555a15e994679cd77e4f2733b29fe1a2d33 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 a48440c46f4e4..b6e7158c9bcf1 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 a44fe820b05945216168eabe475d58f96801dfe5 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 b6e7158c9bcf1..05f90292ff9db 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."); + } } }