Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Socket: don't perform RST close on Dispose when user called Shutdown #41250

Merged
merged 5 commits into from
Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ internal static unsafe InnerSafeCloseSocket Accept(SafeSocketHandle socketHandle
return res;
}

internal unsafe bool TryUnblockSocket(bool abortive)
internal unsafe bool TryUnblockSocket(bool abortive, bool canAbort)
{
// 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.
Expand All @@ -392,7 +392,7 @@ 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)
if (type == (int)SocketType.Stream && canAbort)
{
Interop.Sys.Disconnect(this);
}
Expand All @@ -402,7 +402,7 @@ internal unsafe bool TryUnblockSocket(bool abortive)
}
}

// We've cancelled on-going operations, return true to cause an abortive close.
// We've cancelled on-going operations.
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,10 @@ internal static InnerSafeCloseSocket Accept(SafeSocketHandle socketHandle, byte[
return result;
}

internal unsafe bool TryUnblockSocket(bool abortive)
internal unsafe bool TryUnblockSocket(bool abortive, bool canAbort)
{
// Try to cancel all pending IO.
// If we've canceled operations, we return true to cause an abortive close.
// If we've canceled operations, we return true.
tmds marked this conversation as resolved.
Show resolved Hide resolved
return Interop.Kernel32.CancelIoEx(this, null);
}
}
Expand Down
20 changes: 18 additions & 2 deletions src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ public SafeSocketHandle(IntPtr preexistingHandle, bool ownsHandle)
#if DEBUG
private InnerSafeCloseSocket _innerSocketCopy;
#endif
private bool _sentShutdown;

internal void TrackShutdown(SocketShutdown how)
=> _sentShutdown = _sentShutdown ||
tmds marked this conversation as resolved.
Show resolved Hide resolved
how == SocketShutdown.Send ||
how == SocketShutdown.Both;

public override bool IsInvalid
{
Expand Down Expand Up @@ -171,6 +177,11 @@ internal void CloseAsIs(bool abortive)
Dispose();
if (innerSocket != null)
{
// In case we cancel operations, switch to an abortive close.
// Unless the user requested a normal close using Socket.Shutdown.
bool canceledOperations = false;
bool canAbort = abortive || !_sentShutdown;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble following what abortive vs canAbort means. Can you clarify?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, TryUnblockSocket always performed an abortive (that is :Connection reset by peer) close.
We're changing to take into account if the user has called Socket.Shutdown.
canAbort tells TryUnblockSocket if it can do an abortive close.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, TryUnblockSocket always performed an abortive close.

But TryUnblockSocket was already taking a bool abortive argument. If it always performed an abortive close, what was the purpose of that argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a special case for Sockets that don't have the CLOEXEC flag set:

// Unless we're doing an abortive close, don't touch sockets which don't have the CLOEXEC flag set.
// These may be shared with other processes and we want to avoid disconnecting them.
if (!abortive)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still struggling here, @tmds. Are you saying that both canAbort and abortive mean "do an abortive close", but they communicate that information to different parts of the stack? The names are too close to be meaningful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this comment.

canAbort contains whether a CloseAsIs that was called with abortive: false, can become a an innerSocket.Close with abortive: true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove canAbort and replace it with !hasShutdownSend.


// Wait until it's safe.
SpinWait sw = new SpinWait();
while (!_released)
Expand All @@ -179,11 +190,16 @@ 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, canAbort);
sw.SpinOnce();
}

abortive |= DoReleaseHandle();
canceledOperations |= DoReleaseHandle();

if (canAbort && canceledOperations)
{
abortive = true;
}

innerSocket.Close(abortive);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
36 changes: 36 additions & 0 deletions src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1106,6 +1106,42 @@ public async Task TcpReceiveSendGetsCanceledByDispose(bool receiveOrSend)
}
}, 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
Expand Down