From 73143070366c2ab1a314604fe1df01181637102b Mon Sep 17 00:00:00 2001 From: Aris Rellegue <134557572+arellegue@users.noreply.github.com> Date: Thu, 20 Jul 2023 10:30:00 -0700 Subject: [PATCH] Fix | Take out the ignoreSniOpenTimeout in open connection (#2067) --- .../Microsoft/Data/SqlClient/SNI/SNIProxy.cs | 2 - .../SqlClient/SqlInternalConnectionTds.cs | 4 - .../src/Microsoft/Data/SqlClient/TdsParser.cs | 3 - .../Data/SqlClient/TdsParserStateObject.cs | 1 - .../SqlClient/TdsParserStateObjectManaged.cs | 3 +- .../SqlClient/TdsParserStateObjectNative.cs | 3 +- .../SqlClient/SqlInternalConnectionTds.cs | 6 +- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 3 - .../Data/SqlClient/TdsParserStateObject.cs | 3 +- .../SqlClient/TdsParserSafeHandles.Windows.cs | 16 ++-- .../SqlConnectionBasicTests.cs | 93 +++++++++++++++++++ 11 files changed, 105 insertions(+), 32 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs index f1140fcded..8f101b7bdf 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs @@ -130,7 +130,6 @@ private static bool IsErrorStatus(SecurityStatusPalErrorCode errorCode) /// Create a SNI connection handle /// /// Full server name from connection string - /// Ignore open timeout /// Timer expiration /// Instance name /// SPN @@ -148,7 +147,6 @@ private static bool IsErrorStatus(SecurityStatusPalErrorCode errorCode) /// SNI handle internal static SNIHandle CreateConnectionHandle( string fullServerName, - bool ignoreSniOpenTimeout, long timerExpire, out byte[] instanceName, ref byte[][] spnBuffer, diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index cc727103df..0b745ae01e 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -1543,7 +1543,6 @@ private void LoginNoFailover(ServerInfo serverInfo, AttemptOneLogin(serverInfo, newPassword, newSecurePassword, - !connectionOptions.MultiSubnetFailover, // ignore timeout for SniOpen call unless MSF connectionOptions.MultiSubnetFailover ? intervalTimer : timeout); if (connectionOptions.MultiSubnetFailover && null != ServerProvidedFailOverPartner) @@ -1782,7 +1781,6 @@ TimeoutTimer timeout currentServerInfo, newPassword, newSecurePassword, - false, // Use timeout in SniOpen intervalTimer, withFailover: true ); @@ -1910,7 +1908,6 @@ private void AttemptOneLogin( ServerInfo serverInfo, string newPassword, SecureString newSecurePassword, - bool ignoreSniOpenTimeout, TimeoutTimer timeout, bool withFailover = false) { @@ -1921,7 +1918,6 @@ private void AttemptOneLogin( _parser.Connect(serverInfo, this, - ignoreSniOpenTimeout, timeout.LegacyTimerExpire, ConnectionOptions, withFailover); diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index 1c2e510f46..8c16690149 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -361,7 +361,6 @@ internal void ProcessPendingAck(TdsParserStateObject stateObj) internal void Connect( ServerInfo serverInfo, SqlInternalConnectionTds connHandler, - bool ignoreSniOpenTimeout, long timerExpire, SqlConnectionString connectionOptions, bool withFailover) @@ -445,7 +444,6 @@ internal void Connect( // AD Integrated behaves like Windows integrated when connecting to a non-fedAuth server _physicalStateObj.CreatePhysicalSNIHandle( serverInfo.ExtendedServerName, - ignoreSniOpenTimeout, timerExpire, out instanceName, ref _sniSpnBuffer, @@ -544,7 +542,6 @@ internal void Connect( _physicalStateObj.CreatePhysicalSNIHandle( serverInfo.ExtendedServerName, - ignoreSniOpenTimeout, timerExpire, out instanceName, ref _sniSpnBuffer, true, diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 6be43222ff..1f15345167 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -198,7 +198,6 @@ private void ResetCancelAndProcessAttention() internal abstract void CreatePhysicalSNIHandle( string serverName, - bool ignoreSniOpenTimeout, long timerExpire, out byte[] instanceName, ref byte[][] spnBuffer, diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs index 6d56dbf491..9665d8f188 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs @@ -82,7 +82,6 @@ protected override uint SNIPacketGetData(PacketHandle packet, byte[] inBuff, ref internal override void CreatePhysicalSNIHandle( string serverName, - bool ignoreSniOpenTimeout, long timerExpire, out byte[] instanceName, ref byte[][] spnBuffer, @@ -98,7 +97,7 @@ internal override void CreatePhysicalSNIHandle( string hostNameInCertificate, string serverCertificateFilename) { - SNIHandle? sessionHandle = SNIProxy.CreateConnectionHandle(serverName, ignoreSniOpenTimeout, timerExpire, out instanceName, ref spnBuffer, serverSPN, + SNIHandle? sessionHandle = SNIProxy.CreateConnectionHandle(serverName, timerExpire, out instanceName, ref spnBuffer, serverSPN, flushCache, async, parallel, isIntegratedSecurity, iPAddressPreference, cachedFQDN, ref pendingDNSInfo, tlsFirst, hostNameInCertificate, serverCertificateFilename); diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs index c17a6f9bd4..bf8337cacb 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs @@ -140,7 +140,6 @@ private SNINativeMethodWrapper.ConsumerInfo CreateConsumerInfo(bool async) internal override void CreatePhysicalSNIHandle( string serverName, - bool ignoreSniOpenTimeout, long timerExpire, out byte[] instanceName, ref byte[][] spnBuffer, @@ -199,7 +198,7 @@ internal override void CreatePhysicalSNIHandle( SQLDNSInfo cachedDNSInfo; bool ret = SQLFallbackDNSCache.Instance.GetDNSInfo(cachedFQDN, out cachedDNSInfo); - _sessionHandle = new SNIHandle(myInfo, serverName, spnBuffer[0], ignoreSniOpenTimeout, checked((int)timeout), out instanceName, + _sessionHandle = new SNIHandle(myInfo, serverName, spnBuffer[0], checked((int)timeout), out instanceName, flushCache, !async, fParallel, ipPreference, cachedDNSInfo, hostNameInCertificate); } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index 501c2072ca..aa6a670021 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -1861,7 +1861,6 @@ private void LoginNoFailover(ServerInfo serverInfo, string newPassword, SecureSt AttemptOneLogin(serverInfo, newPassword, newSecurePassword, - !isParallel, // ignore timeout for SniOpen call unless MSF , and TNIR attemptOneLoginTimeout, isFirstTransparentAttempt: isFirstTransparentAttempt, disableTnir: disableTnir); @@ -2137,7 +2136,6 @@ TimeoutTimer timeout currentServerInfo, newPassword, newSecurePassword, - false, // Use timeout in SniOpen intervalTimer, withFailover: true ); @@ -2175,7 +2173,6 @@ TimeoutTimer timeout currentServerInfo, newPassword, newSecurePassword, - false, // Use timeout in SniOpen intervalTimer, withFailover: true ); @@ -2293,7 +2290,7 @@ private void ResolveExtendedServerName(ServerInfo serverInfo, bool aliasLookup, } // Common code path for making one attempt to establish a connection and log in to server. - private void AttemptOneLogin(ServerInfo serverInfo, string newPassword, SecureString newSecurePassword, bool ignoreSniOpenTimeout, TimeoutTimer timeout, bool withFailover = false, bool isFirstTransparentAttempt = true, bool disableTnir = false) + private void AttemptOneLogin(ServerInfo serverInfo, string newPassword, SecureString newSecurePassword, TimeoutTimer timeout, bool withFailover = false, bool isFirstTransparentAttempt = true, bool disableTnir = false) { SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0}, timout={1}[msec], server={2}", ObjectID, timeout.MillisecondsRemaining, serverInfo.ExtendedServerName); @@ -2303,7 +2300,6 @@ private void AttemptOneLogin(ServerInfo serverInfo, string newPassword, SecureSt _parser.Connect(serverInfo, this, - ignoreSniOpenTimeout, timeout.LegacyTimerExpire, ConnectionOptions, withFailover, diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs index 1d29e89fd0..21abbab757 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -494,7 +494,6 @@ internal void ProcessPendingAck(TdsParserStateObject stateObj) internal void Connect(ServerInfo serverInfo, SqlInternalConnectionTds connHandler, - bool ignoreSniOpenTimeout, long timerExpire, SqlConnectionString connectionOptions, bool withFailover, @@ -640,7 +639,6 @@ internal void Connect(ServerInfo serverInfo, _physicalStateObj.CreatePhysicalSNIHandle( serverInfo.ExtendedServerName, - ignoreSniOpenTimeout, timerExpire, out instanceName, _sniSpnBuffer, @@ -746,7 +744,6 @@ internal void Connect(ServerInfo serverInfo, _physicalStateObj.SniContext = SniContext.Snix_Connect; _physicalStateObj.CreatePhysicalSNIHandle( serverInfo.ExtendedServerName, - ignoreSniOpenTimeout, timerExpire, out instanceName, _sniSpnBuffer, diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 122da1982d..5d5ade91f4 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -279,7 +279,6 @@ private SNINativeMethodWrapper.ConsumerInfo CreateConsumerInfo(bool async) internal void CreatePhysicalSNIHandle( string serverName, - bool ignoreSniOpenTimeout, long timerExpire, out byte[] instanceName, byte[] spnBuffer, @@ -318,7 +317,7 @@ internal void CreatePhysicalSNIHandle( _ = SQLFallbackDNSCache.Instance.GetDNSInfo(cachedFQDN, out SQLDNSInfo cachedDNSInfo); - _sessionHandle = new SNIHandle(myInfo, serverName, spnBuffer, ignoreSniOpenTimeout, checked((int)timeout), + _sessionHandle = new SNIHandle(myInfo, serverName, spnBuffer, checked((int)timeout), out instanceName, flushCache, !async, fParallel, transparentNetworkResolutionState, totalTimeout, ipPreference, cachedDNSInfo, hostNameInCertificate); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs index 21ae59c019..8d276cd285 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs @@ -150,7 +150,6 @@ internal SNIHandle( SNINativeMethodWrapper.ConsumerInfo myInfo, string serverName, byte[] spnBuffer, - bool ignoreSniOpenTimeout, int timeout, out byte[] instanceName, bool flushCache, @@ -174,13 +173,14 @@ internal SNIHandle( { _fSync = fSync; instanceName = new byte[256]; // Size as specified by netlibs. - if (ignoreSniOpenTimeout) - { - // UNDONE: ITEM12001110 (DB Mirroring Reconnect) Old behavior of not truly honoring timeout presevered - // for non-failover scenarios to avoid breaking changes as part of a QFE. Consider fixing timeout - // handling in next full release and removing ignoreSniOpenTimeout parameter. - timeout = Timeout.Infinite; // -1 == native SNIOPEN_TIMEOUT_VALUE / INFINITE - } + // Option ignoreSniOpenTimeout is no longer available + //if (ignoreSniOpenTimeout) + //{ + // // UNDONE: ITEM12001110 (DB Mirroring Reconnect) Old behavior of not truly honoring timeout presevered + // // for non-failover scenarios to avoid breaking changes as part of a QFE. Consider fixing timeout + // // handling in next full release and removing ignoreSniOpenTimeout parameter. + // timeout = Timeout.Infinite; // -1 == native SNIOPEN_TIMEOUT_VALUE / INFINITE + //} #if NETFRAMEWORK int transparentNetworkResolutionStateNo = (int)transparentNetworkResolutionState; diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs index 056cd0441b..66dc223c4b 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs @@ -251,6 +251,99 @@ public void ConnectionTestValidCredentialCombination() Assert.Equal(sqlCredential, conn.Credential); } + + [Theory] + [InlineData(60)] + [InlineData(30)] + [InlineData(15)] + [InlineData(10)] + [InlineData(5)] + [InlineData(1)] + public void ConnectionTimeoutTest(int timeout) + { + // Start a server with connection timeout from the inline data. + using TestTdsServer server = TestTdsServer.StartTestServer(false, false, timeout); + using SqlConnection connection = new SqlConnection(server.ConnectionString); + + // Dispose the server to force connection timeout + server.Dispose(); + + // Measure the actual time it took to timeout and compare it with configured timeout + var start = DateTime.Now; + var end = start; + + // Open a connection with the server disposed. + try + { + connection.Open(); + } + catch (Exception) + { + end = DateTime.Now; + } + + // Calculate actual duration of timeout + TimeSpan s = end - start; + // Did not time out? + if (s.TotalSeconds == 0) + Assert.True(s.TotalSeconds == 0); + + // Is actual time out the same as configured timeout or within an additional 3 second threshold because of overhead? + if (s.TotalSeconds > 0) + Assert.True(s.TotalSeconds <= timeout + 3); + } + + [Theory] + [InlineData(60)] + [InlineData(30)] + [InlineData(15)] + [InlineData(10)] + [InlineData(5)] + [InlineData(1)] + public async void ConnectionTimeoutTestAsync(int timeout) + { + // Start a server with connection timeout from the inline data. + using TestTdsServer server = TestTdsServer.StartTestServer(false, false, timeout); + using SqlConnection connection = new SqlConnection(server.ConnectionString); + + // Dispose the server to force connection timeout + server.Dispose(); + + // Measure the actual time it took to timeout and compare it with configured timeout + var start = DateTime.Now; + var end = start; + + // Open a connection with the server disposed. + try + { + await connection.OpenAsync(); + } + catch (Exception) + { + end = DateTime.Now; + } + + // Calculate actual duration of timeout + TimeSpan s = end - start; + // Did not time out? + if (s.TotalSeconds == 0) + Assert.True(s.TotalSeconds == 0); + + // Is actual time out the same as configured timeout or within an additional 3 second threshold because of overhead? + if (s.TotalSeconds > 0) + Assert.True(s.TotalSeconds <= timeout + 3); + } + + [Fact] + public void ConnectionInvalidTimeoutTest() + { + Assert.Throws(() => + { + using TestTdsServer server = TestTdsServer.StartTestServer(false, false, -5); + }); + + } + [Fact] public void ConnectionTestWithCultureTH() {