From 9f2c092366af9ca473bc92bf175dc0f37b87d8af Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 25 Sep 2020 10:06:52 +0200 Subject: [PATCH 1/4] Socket tests: don't retry CanceledByDispose tests on timeout These retries cause masking cases where the operation and dispose Task are never completing. --- .../tests/FunctionalTests/Accept.cs | 36 ++++-- .../tests/FunctionalTests/Connect.cs | 36 ++++-- .../tests/FunctionalTests/SendFile.cs | 51 +++++--- .../tests/FunctionalTests/SendReceive.cs | 110 +++++++++++------- 4 files changed, 152 insertions(+), 81 deletions(-) diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/Accept.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/Accept.cs index 7f0dc3adc14a4..78b172a16bf01 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/Accept.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/Accept.cs @@ -295,7 +295,8 @@ public async Task AcceptGetsCanceledByDispose() // 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)); @@ -330,20 +331,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 { - Assert.Equal(SocketError.OperationAborted, localSocketError); + if (retries-- > 0) + { + continue; + } + + throw; } - }, maxAttempts: 10); + } } [Fact] diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs index 08ecf15f3b925..df26fa35f2f14 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs @@ -127,7 +127,8 @@ public async Task ConnectGetsCanceledByDispose() // 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); @@ -163,20 +164,33 @@ await RetryHelper.ExecuteAsync(async () => disposedException = true; } - if (UsesApm) - { - Assert.Null(localSocketError); - Assert.True(disposedException); - } - else if (UsesSync) + try { - Assert.Equal(SocketError.NotSocket, 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); + } } } diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs index 3a8db0f161ee6..31f3ccea9482d 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs @@ -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) @@ -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] diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs index 50a329e044c2b..9176b99d24a1b 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs @@ -944,7 +944,8 @@ public async Task UdpReceiveGetsCanceledByDispose() // 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 socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp); socket.BindToAnonymousPort(IPAddress.Loopback); @@ -978,20 +979,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 { - Assert.Equal(SocketError.OperationAborted, localSocketError); + if (retries-- > 0) + { + continue; + } + + throw; } - }, maxAttempts: 10); + } } [Theory] @@ -1003,7 +1017,8 @@ public async Task TcpReceiveSendGetsCanceledByDispose(bool receiveOrSend) // 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) @@ -1052,46 +1067,59 @@ await RetryHelper.ExecuteAsync(async () => disposedException = true; } - if (UsesApm) - { - Assert.Null(localSocketError); - Assert.True(disposedException); - } - else if (UsesSync) - { - Assert.Equal(SocketError.ConnectionAborted, localSocketError); - } - else + try { - Assert.Equal(SocketError.OperationAborted, localSocketError); - } + if (UsesApm) + { + Assert.Null(localSocketError); + Assert.True(disposedException); + } + else if (UsesSync) + { + Assert.Equal(SocketError.ConnectionAborted, localSocketError); + } + else + { + Assert.Equal(SocketError.OperationAborted, localSocketError); + } - // On OSX, we're unable to unblock the on-going socket operations and - // perform an abortive close. - if (!(UsesSync && PlatformDetection.IsOSXLike)) - { - SocketError? peerSocketError = null; - var receiveBuffer = new ArraySegment(new byte[4096]); - while (true) + // On OSX, we're unable to unblock the on-going socket operations and + // perform an abortive close. + if (!(UsesSync && PlatformDetection.IsOSXLike)) { - try + SocketError? peerSocketError = null; + var receiveBuffer = new ArraySegment(new byte[4096]); + while (true) { - int received = await ReceiveAsync(socket2, receiveBuffer); - if (received == 0) + try + { + int received = await ReceiveAsync(socket2, receiveBuffer); + if (received == 0) + { + break; + } + } + catch (SocketException se) { + peerSocketError = se.SocketErrorCode; break; } } - catch (SocketException se) - { - peerSocketError = se.SocketErrorCode; - break; - } + Assert.Equal(SocketError.ConnectionReset, peerSocketError); + } + break; + } + catch + { + if (retries-- > 0) + { + continue; } - Assert.Equal(SocketError.ConnectionReset, peerSocketError); + + throw; } } - }, maxAttempts: 10); + } } [Fact] From 37176da850ea52041b528dc95db5e022352258fc Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 28 Sep 2020 15:23:06 +0200 Subject: [PATCH 2/4] print g_config_specified_ciphersuites --- .../Native/Unix/System.Security.Cryptography.Native/pal_ssl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c index b53abd73f235c..44fb1f317328f 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c @@ -11,6 +11,7 @@ #include #include #include +#include c_static_assert(PAL_SSL_ERROR_NONE == SSL_ERROR_NONE); c_static_assert(PAL_SSL_ERROR_SSL == SSL_ERROR_SSL); @@ -118,6 +119,8 @@ static void DetectCiphersuiteConfiguration() g_config_specified_ciphersuites = 1; #endif + write(0, "config\n", 7); + write(0, g_config_specified_ciphersuites == 1 ? "1\n" : "0\n", 2); } void CryptoNative_EnsureLibSslInitialized() From b7b8d2cd9c9bc7c05b44b0f606a29b7ce6611a41 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 1 Oct 2020 17:01:06 +0200 Subject: [PATCH 3/4] Move connection timeout retry --- .../System.Net.Sockets/tests/FunctionalTests/Connect.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs index df26fa35f2f14..fd84bcfb664dd 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs @@ -154,9 +154,6 @@ public async Task ConnectGetsCanceledByDispose() } catch (SocketException se) { - // On connection timeout, retry. - Assert.NotEqual(SocketError.TimedOut, se.SocketErrorCode); - localSocketError = se.SocketErrorCode; } catch (ObjectDisposedException) @@ -166,6 +163,9 @@ public async Task ConnectGetsCanceledByDispose() try { + // On connection timeout, retry. + Assert.NotEqual(SocketError.TimedOut, localSocketError); + if (UsesApm) { Assert.Null(localSocketError); From ee3dd1944f3d237108b7d29f1d79de753b5352e8 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Oct 2020 19:21:34 +0200 Subject: [PATCH 4/4] Undo unrelated changes --- .../Native/Unix/System.Security.Cryptography.Native/pal_ssl.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c index 44fb1f317328f..b53abd73f235c 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c @@ -11,7 +11,6 @@ #include #include #include -#include c_static_assert(PAL_SSL_ERROR_NONE == SSL_ERROR_NONE); c_static_assert(PAL_SSL_ERROR_SSL == SSL_ERROR_SSL); @@ -119,8 +118,6 @@ static void DetectCiphersuiteConfiguration() g_config_specified_ciphersuites = 1; #endif - write(0, "config\n", 7); - write(0, g_config_specified_ciphersuites == 1 ? "1\n" : "0\n", 2); } void CryptoNative_EnsureLibSslInitialized()