From 9813ce3f247c129be858d66bb8affe109358a94c Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 4 Apr 2023 19:20:18 -0700 Subject: [PATCH 1/4] Fix Transient fault handling issue with OpenAsync --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 4 +- .../Microsoft/Data/SqlClient/SqlConnection.cs | 4 +- .../SqlConnectionBasicTests.cs | 86 ++++++++++++++++++- 3 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs index f7e79da653..089eb956e5 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1856,7 +1856,7 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec } } - _applyTransientFaultHandling = (!overrides.HasFlag(SqlConnectionOverrides.OpenWithoutRetry) && retry == null && connectionOptions != null && connectionOptions.ConnectRetryCount > 0); + _applyTransientFaultHandling = (!overrides.HasFlag(SqlConnectionOverrides.OpenWithoutRetry) && connectionOptions != null && connectionOptions.ConnectRetryCount > 0); if (connectionOptions != null && (connectionOptions.Authentication == SqlAuthenticationMethod.SqlPassword || @@ -1885,7 +1885,7 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec // does not require GC.KeepAlive(this) because of ReRegisterForFinalize below. // Set future transient fault handling based on connection options - _applyTransientFaultHandling = (retry == null && connectionOptions != null && connectionOptions.ConnectRetryCount > 0); + _applyTransientFaultHandling = connectionOptions != null && connectionOptions.ConnectRetryCount > 0; var tdsInnerConnection = (SqlInternalConnectionTds)InnerConnection; diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs index 681faaaf15..83233d4ac3 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2068,7 +2068,7 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec bool result = false; - _applyTransientFaultHandling = (!overrides.HasFlag(SqlConnectionOverrides.OpenWithoutRetry) && retry == null && connectionOptions != null && connectionOptions.ConnectRetryCount > 0); + _applyTransientFaultHandling = (!overrides.HasFlag(SqlConnectionOverrides.OpenWithoutRetry) && connectionOptions != null && connectionOptions.ConnectRetryCount > 0); if (connectionOptions != null && (connectionOptions.Authentication == SqlAuthenticationMethod.SqlPassword || @@ -2111,7 +2111,7 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec } // Set future transient fault handling based on connection options - _applyTransientFaultHandling = (retry == null && connectionOptions != null && connectionOptions.ConnectRetryCount > 0); + _applyTransientFaultHandling = connectionOptions != null && connectionOptions.ConnectRetryCount > 0; return result; } diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs index 039bf2f766..98c2aef894 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs @@ -7,6 +7,7 @@ using System.Data.Common; using System.Reflection; using System.Security; +using System.Threading.Tasks; using Microsoft.SqlServer.TDS.Servers; using Xunit; @@ -34,6 +35,33 @@ public void IntegratedAuthConnectionTest() connection.Open(); } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArmProcess))] + [InlineData(40613)] + [InlineData(42108)] + [InlineData(42109)] + [PlatformSpecific(TestPlatforms.Windows)] + public async Task TransientFaultTestAsync(uint errorCode) + { + using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(true, true, errorCode); + SqlConnectionStringBuilder builder = new() + { + DataSource = "localhost," + server.Port, + IntegratedSecurity = true, + Encrypt = SqlConnectionEncryptOption.Optional + }; + + using SqlConnection connection = new(builder.ConnectionString); + try + { + await connection.OpenAsync(); + Assert.Equal(ConnectionState.Open, connection.State); + } + catch (Exception e) + { + Assert.False(true, e.Message); + } + } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArmProcess))] [InlineData(40613)] [InlineData(42108)] @@ -57,14 +85,64 @@ public void TransientFaultTest(uint errorCode) } catch (Exception e) { - if (null != connection) - { - Assert.Equal(ConnectionState.Closed, connection.State); - } Assert.False(true, e.Message); } } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArmProcess))] + [InlineData(40613)] + [InlineData(42108)] + [InlineData(42109)] + [PlatformSpecific(TestPlatforms.Windows)] + public async Task TransientFaultDisabledTestAsync(uint errorCode) + { + using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(false, true, errorCode); + SqlConnectionStringBuilder builder = new() + { + DataSource = "localhost," + server.Port, + IntegratedSecurity = true, + Encrypt = SqlConnectionEncryptOption.Optional + }; + + using SqlConnection connection = new(builder.ConnectionString); + try + { + await connection.OpenAsync(); + Assert.False(true, "Connection should not have opened."); + } + catch + { + Assert.Equal(ConnectionState.Closed, connection.State); + } + } + + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArmProcess))] + [InlineData(40613)] + [InlineData(42108)] + [InlineData(42109)] + [PlatformSpecific(TestPlatforms.Windows)] + public void TransientFaultDisabledTest(uint errorCode) + { + using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(false, true, errorCode); + SqlConnectionStringBuilder builder = new() + { + DataSource = "localhost," + server.Port, + IntegratedSecurity = true, + Encrypt = SqlConnectionEncryptOption.Optional + }; + + using SqlConnection connection = new(builder.ConnectionString); + try + { + connection.Open(); + Assert.False(true, "Connection should not have opened."); + } + catch + { + Assert.Equal(ConnectionState.Closed, connection.State); + } + } + [Fact] public void SqlConnectionDbProviderFactoryTest() { From d102b62c9bdc5534141a3aa44a70d430b48f69e9 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 4 Apr 2023 20:15:07 -0700 Subject: [PATCH 2/4] Fix test --- .../SqlConnectionBasicTests.cs | 20 +++++++++++++------ .../TDS.Servers/TransientFaultTDSServer.cs | 1 + 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs index 98c2aef894..1456c11c3d 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs @@ -96,11 +96,12 @@ public void TransientFaultTest(uint errorCode) [PlatformSpecific(TestPlatforms.Windows)] public async Task TransientFaultDisabledTestAsync(uint errorCode) { - using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(false, true, errorCode); + using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(true, true, errorCode); SqlConnectionStringBuilder builder = new() { DataSource = "localhost," + server.Port, IntegratedSecurity = true, + ConnectRetryCount = 0, Encrypt = SqlConnectionEncryptOption.Optional }; @@ -110,9 +111,12 @@ public async Task TransientFaultDisabledTestAsync(uint errorCode) await connection.OpenAsync(); Assert.False(true, "Connection should not have opened."); } - catch + catch (SqlException e) { - Assert.Equal(ConnectionState.Closed, connection.State); + if (e.Class == 20) // FATAL Error, should result in closed connection. + { + Assert.Equal(ConnectionState.Closed, connection.State); + } } } @@ -123,11 +127,12 @@ public async Task TransientFaultDisabledTestAsync(uint errorCode) [PlatformSpecific(TestPlatforms.Windows)] public void TransientFaultDisabledTest(uint errorCode) { - using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(false, true, errorCode); + using TransientFaultTDSServer server = TransientFaultTDSServer.StartTestServer(true, true, errorCode); SqlConnectionStringBuilder builder = new() { DataSource = "localhost," + server.Port, IntegratedSecurity = true, + ConnectRetryCount = 0, Encrypt = SqlConnectionEncryptOption.Optional }; @@ -137,9 +142,12 @@ public void TransientFaultDisabledTest(uint errorCode) connection.Open(); Assert.False(true, "Connection should not have opened."); } - catch + catch (SqlException e) { - Assert.Equal(ConnectionState.Closed, connection.State); + if (e.Class == 20) // FATAL Error, should result in closed connection. + { + Assert.Equal(ConnectionState.Closed, connection.State); + } } } diff --git a/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientFaultTDSServer.cs b/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientFaultTDSServer.cs index 419f7e5d24..eda4de8e2a 100644 --- a/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientFaultTDSServer.cs +++ b/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientFaultTDSServer.cs @@ -146,6 +146,7 @@ private void Dispose(bool isDisposing) if (isDisposing) { _endpoint?.Stop(); + RequestCounter = 0; } } } From 0018f1962fdc1c5d789775942c54fab0d16fe273 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Mon, 10 Apr 2023 10:46:06 -0700 Subject: [PATCH 3/4] Use asserts instead --- .../FunctionalTests/SqlConnectionBasicTests.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs index 1456c11c3d..25ef3eb736 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs @@ -113,10 +113,9 @@ public async Task TransientFaultDisabledTestAsync(uint errorCode) } catch (SqlException e) { - if (e.Class == 20) // FATAL Error, should result in closed connection. - { - Assert.Equal(ConnectionState.Closed, connection.State); - } + // FATAL Error, should result in closed connection. + Assert.Equal(20, e.Class); + Assert.Equal(ConnectionState.Closed, connection.State); } } @@ -144,10 +143,9 @@ public void TransientFaultDisabledTest(uint errorCode) } catch (SqlException e) { - if (e.Class == 20) // FATAL Error, should result in closed connection. - { - Assert.Equal(ConnectionState.Closed, connection.State); - } + // FATAL Error, should result in closed connection. + Assert.Equal(20, e.Class); + Assert.Equal(ConnectionState.Closed, connection.State); } } From 80c4096ba048cf1a92b11761ea9bfc20d71d60c6 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Date: Thu, 13 Apr 2023 17:43:24 -0700 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com> --- .../tests/FunctionalTests/SqlConnectionBasicTests.cs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs index 25ef3eb736..e61bdfe4ac 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs @@ -51,15 +51,8 @@ public async Task TransientFaultTestAsync(uint errorCode) }; using SqlConnection connection = new(builder.ConnectionString); - try - { - await connection.OpenAsync(); - Assert.Equal(ConnectionState.Open, connection.State); - } - catch (Exception e) - { - Assert.False(true, e.Message); - } + await connection.OpenAsync(); + Assert.Equal(ConnectionState.Open, connection.State); } [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArmProcess))]