From 4826532b12be39f4b6634d73a13ddb6a2e346697 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 16 Oct 2019 03:28:17 +0200 Subject: [PATCH] Socket: don't perform RST close on Dispose when user called Shutdown (#41250) * Socket: don't perform RST close on Dispose when user called Shutdown * Add test * PR feedback * Remove confusing 'canAbort' local * Improve comments --- .../Net/Sockets/SafeSocketHandle.Unix.cs | 8 +++-- .../Net/Sockets/SafeSocketHandle.Windows.cs | 4 +-- .../System/Net/Sockets/SafeSocketHandle.cs | 23 ++++++++++-- .../src/System/Net/Sockets/SocketPal.Unix.cs | 2 ++ .../System/Net/Sockets/SocketPal.Windows.cs | 1 + .../tests/FunctionalTests/SendReceive.cs | 36 +++++++++++++++++++ 6 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs b/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs index 2972b63bc1eb..003c3d857929 100644 --- a/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs +++ b/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs @@ -366,7 +366,8 @@ internal static unsafe InnerSafeCloseSocket Accept(SafeSocketHandle socketHandle return res; } - internal unsafe bool TryUnblockSocket(bool abortive) + /// Returns whether operations were canceled. + internal unsafe bool TryUnblockSocket(bool abortive, bool hasShutdownSend) { // Calling 'close' on a socket that has pending blocking calls (e.g. recv, send, accept, ...) // may block indefinitely. This is a best-effort attempt to not get blocked and make those operations return. @@ -392,7 +393,9 @@ internal unsafe bool TryUnblockSocket(bool abortive) Interop.Error err = Interop.Sys.GetSockOpt(this, SocketOptionLevel.Socket, SocketOptionName.Type, (byte*)&type, &optLen); if (err == Interop.Error.SUCCESS) { - if (type == (int)SocketType.Stream) + // For TCP (SocketType.Stream), perform an abortive close. + // Unless the user requested a normal close using Socket.Shutdown. + if (type == (int)SocketType.Stream && !hasShutdownSend) { Interop.Sys.Disconnect(this); } @@ -402,7 +405,6 @@ internal unsafe bool TryUnblockSocket(bool abortive) } } - // We've cancelled on-going operations, return true to cause an abortive close. return true; } } diff --git a/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs b/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs index 47da6a2ad7fc..0c7b5d751e9c 100644 --- a/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs +++ b/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs @@ -236,10 +236,10 @@ internal static InnerSafeCloseSocket Accept(SafeSocketHandle socketHandle, byte[ return result; } - internal unsafe bool TryUnblockSocket(bool abortive) + /// Returns whether operations were canceled. + internal unsafe bool TryUnblockSocket(bool abortive, bool hasShutdownSend) { // Try to cancel all pending IO. - // If we've canceled operations, we return true to cause an abortive close. return Interop.Kernel32.CancelIoEx(this, null); } } diff --git a/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs b/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs index 0e146df7c23e..6b2f884f7540 100644 --- a/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs +++ b/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs @@ -37,6 +37,16 @@ private SafeSocketHandle() : base(true) { } #if DEBUG private InnerSafeCloseSocket _innerSocketCopy; #endif + private bool _hasShutdownSend; + + internal void TrackShutdown(SocketShutdown how) + { + if (how == SocketShutdown.Send || + how == SocketShutdown.Both) + { + _hasShutdownSend = true; + } + } public override bool IsInvalid { @@ -171,6 +181,8 @@ internal void CloseAsIs(bool abortive) Dispose(); if (innerSocket != null) { + bool canceledOperations = false; + // Wait until it's safe. SpinWait sw = new SpinWait(); while (!_released) @@ -179,11 +191,18 @@ internal void CloseAsIs(bool abortive) // 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. - abortive |= innerSocket.TryUnblockSocket(abortive); + canceledOperations |= innerSocket.TryUnblockSocket(abortive, _hasShutdownSend); sw.SpinOnce(); } - abortive |= DoReleaseHandle(); + canceledOperations |= DoReleaseHandle(); + + // In case we cancel operations, switch to an abortive close. + // Unless the user requested a normal close using Socket.Shutdown. + if (canceledOperations && !_hasShutdownSend) + { + abortive = true; + } innerSocket.Close(abortive); } diff --git a/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs b/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs index bf1fec6176d3..e4067c0f5a5a 100644 --- a/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs +++ b/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs @@ -1535,6 +1535,7 @@ public static SocketError Shutdown(SafeSocketHandle handle, bool isConnected, bo Interop.Error err = Interop.Sys.Shutdown(handle, how); if (err == Interop.Error.SUCCESS) { + handle.TrackShutdown(how); return SocketError.Success; } @@ -1543,6 +1544,7 @@ public static SocketError Shutdown(SafeSocketHandle handle, bool isConnected, bo // has reached the CLOSE state. Ignoring the error matches Winsock behavior. if (err == Interop.Error.ENOTCONN && (isConnected || isDisconnected)) { + handle.TrackShutdown(how); return SocketError.Success; } diff --git a/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs b/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs index a68bb710d553..f33cc45d04a6 100644 --- a/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs +++ b/src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs @@ -891,6 +891,7 @@ public static SocketError Shutdown(SafeSocketHandle handle, bool isConnected, bo SocketError err = Interop.Winsock.shutdown(handle, (int)how); if (err != SocketError.SocketError) { + handle.TrackShutdown(how); return SocketError.Success; } diff --git a/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs b/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs index c1b59ef8b08d..0e1c350a1ebc 100644 --- a/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs +++ b/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs @@ -1106,6 +1106,42 @@ await RetryHelper.ExecuteAsync(async () => } }, maxAttempts: 10); } + + [Fact] + public async Task TcpPeerReceivesFinOnShutdownWithPendingData() + { + // We try this a couple of times to deal with a timing race: if the Dispose happens + // before the send is started, no data is sent. + int msDelay = 100; + byte[] hugeBuffer = new byte[100_000_000]; + byte[] receiveBuffer = new byte[1024]; + await RetryHelper.ExecuteAsync(async () => + { + (Socket socket1, Socket socket2) = CreateConnectedSocketPair(); + using (socket1) + using (socket2) + { + // socket1: send a huge amount of data, then Shutdown and Dispose before the peer starts reading. + Task sendTask = SendAsync(socket1, hugeBuffer); + // Wait a little so the operation is started. + await Task.Delay(msDelay); + msDelay *= 2; + socket1.Shutdown(SocketShutdown.Both); + socket1.Dispose(); + + // socket2: read until FIN. + int receivedTotal = 0; + int received; + do + { + received = await ReceiveAsync(socket2, receiveBuffer); + receivedTotal += received; + } while (received != 0); + + Assert.NotEqual(0, receivedTotal); + } + }, maxAttempts: 10); + } } public class SendReceive