Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RHEL7 socket dispose hang, and extend coverage #43409

Merged
merged 27 commits into from
Oct 26, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
9f2c092
Socket tests: don't retry CanceledByDispose tests on timeout
tmds Sep 25, 2020
37176da
print g_config_specified_ciphersuites
tmds Sep 28, 2020
b7b8d2c
Move connection timeout retry
tmds Oct 1, 2020
ee3dd19
Undo unrelated changes
tmds Oct 6, 2020
48a69c4
Merge branch 'canceledbydispose_timeout' into af/disposecancel
antonfirsov Oct 12, 2020
512edf5
test cancellation also with IPV6 and Dual Mode
antonfirsov Oct 12, 2020
80501f9
do not use 1.1.1.1 and similar
antonfirsov Oct 12, 2020
4d83e73
Revert "do not use 1.1.1.1 and similar"
antonfirsov Oct 12, 2020
ba5df5e
do not test IPV6 for now
antonfirsov Oct 12, 2020
b301bb0
fall back to shutdown, if connect(AF_UNSPEC) fails
antonfirsov Oct 12, 2020
6158460
TcpReceiveSendGetsCanceledByDispose timeout handling
antonfirsov Oct 12, 2020
b77d31f
update tests
antonfirsov Oct 14, 2020
1da5006
OuterLoop test using 1.1.1.1
antonfirsov Oct 14, 2020
5b9b879
change CreateConnectedSocketPair API
antonfirsov Oct 14, 2020
102561a
do not use SkipTestException
antonfirsov Oct 14, 2020
f591786
CI tweaks
antonfirsov Oct 16, 2020
6909cc8
fix Unix build
antonfirsov Oct 22, 2020
8a3ce60
fix TcpReceiveSendGetsCanceledByDispose for RHEL7
antonfirsov Oct 22, 2020
800d910
actually fix TcpReceiveSendGetsCanceledByDispose
antonfirsov Oct 22, 2020
b371ae7
RetryHelper: add retryWhen argument
antonfirsov Oct 22, 2020
720c522
AcceptGetsCanceledByDispose: use RetryHelper again
antonfirsov Oct 22, 2020
412a1df
ConnectGetsCanceledByDispose: use RetryHelper again
antonfirsov Oct 22, 2020
86dca50
SyncSendFileGetsCanceledByDispose: use RetryHelper again
antonfirsov Oct 22, 2020
5b109e3
SendReceive: use RetryHelper again
antonfirsov Oct 22, 2020
9842c81
remove empty line
antonfirsov Oct 22, 2020
1769820
fix build
antonfirsov Oct 22, 2020
7475c15
add missing <param> docs
antonfirsov Oct 22, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,21 @@ public static void ForceNonBlocking(this Socket socket, bool force)
}
}

public static (Socket, Socket) CreateConnectedSocketPair()
public static (Socket client, Socket server) CreateConnectedSocketPair() => CreateConnectedSocketPair(IPAddress.Loopback, false);

public static (Socket client, Socket server) CreateConnectedSocketPair(IPAddress serverAddress, bool dualModeClient)
antonfirsov marked this conversation as resolved.
Show resolved Hide resolved
{
using Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
using Socket listener = new Socket(serverAddress.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
listener.Bind(new IPEndPoint(serverAddress, 0));
listener.Listen(1);

Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
client.Connect(listener.LocalEndPoint);
IPEndPoint connectTo = (IPEndPoint)listener.LocalEndPoint;
if (dualModeClient) connectTo = new IPEndPoint(connectTo.Address.MapToIPv6(), connectTo.Port);


Socket client = new Socket(connectTo.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
if (dualModeClient) client.DualMode = true;
client.Connect(connectTo);
Socket server = listener.Accept();

return (client, server);
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/Native/Unix/System.Native/pal_networking.c
Original file line number Diff line number Diff line change
Expand Up @@ -3085,6 +3085,11 @@ int32_t SystemNative_Disconnect(intptr_t socket)
addr.sa_family = AF_UNSPEC;

err = connect(fd, &addr, sizeof(addr));
if (err != 0)
{
// On some older kernels connect(AF_UNSPEC) may fail. Fall back to shutdown in these cases:
Copy link
Member

Choose a reason for hiding this comment

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

connect(AF_UNSPEC) has worked since old kernels. It's an issue in SELinux that caused this to fail. That's why it is affecting SELinux based distros. You don't need to update the comment, this is background info.

err = shutdown(fd, SHUT_RDWR);
}
#elif HAVE_DISCONNECTX
// disconnectx causes a FIN close on OSX. It's the best we can do.
err = disconnectx(fd, SAE_ASSOCID_ANY, SAE_CONNID_ANY);
Expand Down
53 changes: 38 additions & 15 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/Accept.cs
Original file line number Diff line number Diff line change
Expand Up @@ -289,16 +289,26 @@ public async Task AcceptAsync_MultipleAcceptsThenDispose_AcceptsThrowAfterDispos
}
}

[Fact]
public async Task AcceptGetsCanceledByDispose()
public static readonly TheoryData<IPAddress> AcceptGetsCanceledByDispose_Data = new TheoryData<IPAddress>
{
{ IPAddress.Loopback },
{ IPAddress.IPv6Loopback },
{ IPAddress.Loopback.MapToIPv6() }
};

[Theory]
[MemberData(nameof(AcceptGetsCanceledByDispose_Data))]
public async Task AcceptGetsCanceledByDispose(IPAddress loopback)
{
// We try this a couple of times to deal with a timing race: if the Dispose happens
// before the operation is started, we won't see a SocketException.
int msDelay = 100;
await RetryHelper.ExecuteAsync(async () =>
int retries = 10;
while (true)
{
var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
var listener = new Socket(loopback.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
if (loopback.IsIPv4MappedToIPv6) listener.DualMode = true;
listener.Bind(new IPEndPoint(loopback, 0));
listener.Listen(1);

Task acceptTask = AcceptAsync(listener);
Expand Down Expand Up @@ -330,20 +340,33 @@ await RetryHelper.ExecuteAsync(async () =>
disposedException = true;
}

if (UsesApm)
{
Assert.Null(localSocketError);
Assert.True(disposedException);
}
else if (UsesSync)
try
{
Assert.Equal(SocketError.Interrupted, localSocketError);
if (UsesApm)
{
Assert.Null(localSocketError);
Assert.True(disposedException);
}
else if (UsesSync)
{
Assert.Equal(SocketError.Interrupted, localSocketError);
}
else
{
Assert.Equal(SocketError.OperationAborted, localSocketError);
}
break;
}
else
catch
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we catch a specific exception here, like some XUnit exception class? Seems like that would be safer and clearer. Similarly below.

{
Assert.Equal(SocketError.OperationAborted, localSocketError);
if (retries-- > 0)
{
continue;
}

throw;
}
}, maxAttempts: 10);
}
}

[Fact]
Expand Down
60 changes: 42 additions & 18 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,29 @@ public async Task Connect_AfterDisconnect_Fails()
}
}

[Fact]
public static readonly TheoryData<IPAddress> ConnectGetsCanceledByDispose_Data = new TheoryData<IPAddress>
{
{ IPAddress.Parse("1.1.1.1") },
// TODO: Figure out how to test this with vanilla IPV6
{ IPAddress.Parse("1.1.1.1").MapToIPv6() },
};

[OuterLoop("Uses external server")]
[Theory]
[MemberData(nameof(ConnectGetsCanceledByDispose_Data))]
[PlatformSpecific(~(TestPlatforms.OSX | TestPlatforms.FreeBSD))] // Not supported on BSD like OSes.
public async Task ConnectGetsCanceledByDispose()
public async Task ConnectGetsCanceledByDispose(IPAddress address)
{
// We try this a couple of times to deal with a timing race: if the Dispose happens
// before the operation is started, we won't see a SocketException.
int msDelay = 100;
await RetryHelper.ExecuteAsync(async () =>
int retries = 10;
while (true)
{
var client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
var client = new Socket(address.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
if (address.IsIPv4MappedToIPv6) client.DualMode = true;

Task connectTask = ConnectAsync(client, new IPEndPoint(IPAddress.Parse("1.1.1.1"), 23));
Task connectTask = ConnectAsync(client, new IPEndPoint(address, 23));

// Wait a little so the operation is started.
await Task.Delay(msDelay);
Expand All @@ -153,30 +164,43 @@ await RetryHelper.ExecuteAsync(async () =>
}
catch (SocketException se)
{
// On connection timeout, retry.
Assert.NotEqual(SocketError.TimedOut, se.SocketErrorCode);

localSocketError = se.SocketErrorCode;
}
catch (ObjectDisposedException)
{
disposedException = true;
}

if (UsesApm)
{
Assert.Null(localSocketError);
Assert.True(disposedException);
}
else if (UsesSync)
try
{
Assert.Equal(SocketError.NotSocket, localSocketError);
// On connection timeout, retry.
Assert.NotEqual(SocketError.TimedOut, localSocketError);

if (UsesApm)
{
Assert.Null(localSocketError);
Assert.True(disposedException);
}
else if (UsesSync)
{
Assert.Equal(SocketError.NotSocket, localSocketError);
}
else
{
Assert.Equal(SocketError.OperationAborted, localSocketError);
}
break;
}
else
catch
{
Assert.Equal(SocketError.OperationAborted, localSocketError);
if (retries-- > 0)
{
continue;
}

throw;
}
}, maxAttempts: 10);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ public void InlineSocketContinuations()
await new SendReceiveEap(null).SendRecv_Stream_TCP(IPAddress.Loopback, useMultipleBuffers: false);
await new SendReceiveEap(null).SendRecv_Stream_TCP_MultipleConcurrentReceives(IPAddress.Loopback, useMultipleBuffers: false);
await new SendReceiveEap(null).SendRecv_Stream_TCP_MultipleConcurrentSends(IPAddress.Loopback, useMultipleBuffers: false);
await new SendReceiveEap(null).TcpReceiveSendGetsCanceledByDispose(receiveOrSend: true);
await new SendReceiveEap(null).TcpReceiveSendGetsCanceledByDispose(receiveOrSend: false);
await new SendReceiveEap(null).TcpReceiveSendGetsCanceledByDispose(receiveOrSend: true, serverAddress: IPAddress.Loopback, dualModeClient: false);
await new SendReceiveEap(null).TcpReceiveSendGetsCanceledByDispose(receiveOrSend: false, serverAddress: IPAddress.Loopback, dualModeClient: false);
}, options).Dispose();
}
}
Expand Down
51 changes: 33 additions & 18 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,8 @@ public async Task SyncSendFileGetsCanceledByDispose()
// before the operation is started, the peer won't see a ConnectionReset SocketException and we won't
// see a SocketException either.
int msDelay = 100;
await RetryHelper.ExecuteAsync(async () =>
int retries = 10;
while (true)
{
(Socket socket1, Socket socket2) = SocketTestExtensions.CreateConnectedSocketPair();
using (socket2)
Expand Down Expand Up @@ -328,34 +329,48 @@ await RetryHelper.ExecuteAsync(async () =>
}
catch (ObjectDisposedException)
{ }
Assert.Equal(SocketError.ConnectionAborted, localSocketError);

// On OSX, we're unable to unblock the on-going socket operations and
// perform an abortive close.
if (!PlatformDetection.IsOSXLike)
try
{
SocketError? peerSocketError = null;
var receiveBuffer = new byte[4096];
while (true)
Assert.Equal(SocketError.ConnectionAborted, localSocketError);

// On OSX, we're unable to unblock the on-going socket operations and
// perform an abortive close.
if (!PlatformDetection.IsOSXLike)
{
try
SocketError? peerSocketError = null;
var receiveBuffer = new byte[4096];
while (true)
{
int received = socket2.Receive(receiveBuffer);
if (received == 0)
try
{
int received = socket2.Receive(receiveBuffer);
if (received == 0)
{
break;
}
}
catch (SocketException se)
{
peerSocketError = se.SocketErrorCode;
break;
}
}
catch (SocketException se)
{
peerSocketError = se.SocketErrorCode;
break;
}
Assert.Equal(SocketError.ConnectionReset, peerSocketError);
}
Assert.Equal(SocketError.ConnectionReset, peerSocketError);
break;
}
catch
{
if (retries-- > 0)
{
continue;
}

throw;
}
}
}, maxAttempts: 10);
}
}

[OuterLoop]
Expand Down
Loading