Fix | Take out the ignoreSniOpenTimeout in open connection (#2067)

arellegue · web-flow · commit 73143070366c · 2023-07-20T10:30:00.000-07:00
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
@@ -130,7 +130,6 @@ private static bool IsErrorStatus(SecurityStatusPalErrorCode errorCode)
         /// Create a SNI connection handle
         /// </summary>
         /// <param name="fullServerName">Full server name from connection string</param>
-        /// <param name="ignoreSniOpenTimeout">Ignore open timeout</param>
         /// <param name="timerExpire">Timer expiration</param>
         /// <param name="instanceName">Instance name</param>
         /// <param name="spnBuffer">SPN</param>
@@ -148,7 +147,6 @@ private static bool IsErrorStatus(SecurityStatusPalErrorCode errorCode)
         /// <returns>SNI handle</returns>
         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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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("<sc.SqlInternalConnectionTds.AttemptOneLogin|ADV> {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
@@ -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
@@ -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
@@ -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
@@ -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<ArgumentException>(() =>
+            {
+                using TestTdsServer server = TestTdsServer.StartTestServer(false, false, -5);
+            });
+
+        }
+
         [Fact]
         public void ConnectionTestWithCultureTH()
         {
