Fixed connection retry behavior when connectRetryCount is set to a value greater than 1

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

@@ -1994,7 +1994,7 @@ Connection connect(Properties propsIn, SQLServerPooledConnection pooledConnectio
                     if (0 == connectRetryCount) {
                         // connection retry disabled
                         throw e;
-                    } else if (connectRetryAttempt++ > connectRetryCount) {
+                    } else if (connectRetryAttempt++ >= connectRetryCount) {
                         // maximum connection retry count reached
                         if (connectionlogger.isLoggable(Level.FINE)) {
                             connectionlogger.fine("Connection failed. Maximum connection retry count "
@@ -2027,7 +2027,10 @@ Connection connect(Properties propsIn, SQLServerPooledConnection pooledConnectio
                                     + connectRetryInterval + ")s before retry.");
                         }
 
-                        sleepForInterval(TimeUnit.SECONDS.toMillis(connectRetryInterval));
+                        if (connectRetryAttempt > 1) {
+                            // We do not sleep for first retry; first retry is immediate
+                            sleepForInterval(TimeUnit.SECONDS.toMillis(connectRetryInterval));
+                        }
                     }
                 }
             }

src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java

@@ -454,17 +454,26 @@ public void testConnectionPoolGetTwice() throws SQLException {
         }
     }
 
+    /**
+     * Runs the `testConnectCountInLoginAndCorrectRetryCount` test several times with different values of
+     * connectRetryCount.
+     */
+    @Test
+    public void testConnectCountInLoginAndCorrectRetryCountForMultipleValues() {
+        testConnectCountInLoginAndCorrectRetryCount(0);
+        testConnectCountInLoginAndCorrectRetryCount(1);
+        testConnectCountInLoginAndCorrectRetryCount(2);
+    }
+
     /**
      * Tests whether connectRetryCount and connectRetryInterval are properly respected in the login loop. As well, tests
      * that connection is retried the proper number of times.
      */
-    @Test
-    public void testConnectCountInLoginAndCorrectRetryCount() {
+    private void testConnectCountInLoginAndCorrectRetryCount(int connectRetryCount) {
         long timerStart = 0;
 
-        int connectRetryCount = 0;
         int connectRetryInterval = 60;
-        int longLoginTimeout = loginTimeOutInSeconds * 3; // 90 seconds
+        int longLoginTimeout = loginTimeOutInSeconds * 9; // 90 seconds
 
         try {
             SQLServerDataSource ds = new SQLServerDataSource();
@@ -490,6 +499,15 @@ public void testConnectCountInLoginAndCorrectRetryCount() {
 
             // Maximum is unknown, but is needs to be less than longLoginTimeout or else this is an issue.
             assertTrue(totalTime < (longLoginTimeout * 1000L), TestResource.getResource("R_executionTooLong"));
+
+            // We should at least take as long as the retry interval between all retries past the first.
+            // Of the above acceptable errors (R_cannotOpenDatabase, R_loginFailedMI, R_MInotAvailable), only
+            // R_cannotOpenDatabase is transient, and can be used to measure multiple retries with retry interval. The
+            // others will exit before they have a chance to wait, and min will be too low.
+            if (e.getMessage().contains(TestResource.getResource("R_cannotOpenDatabase"))) {
+                int minTimeInSecs = connectRetryInterval * (connectRetryCount - 1);
+                assertTrue(totalTime > (minTimeInSecs * 1000L), TestResource.getResource("R_executionNotLong"));
+            }
         }
     }
 
@@ -800,9 +818,9 @@ public void testIncorrectDatabase() throws SQLException {
         } catch (Exception e) {
             assertTrue(
                     e.getMessage().contains(TestResource.getResource("R_cannotOpenDatabase"))
-                            || (TestUtils.getProperty(connectionString, "msiClientId") != null
+                                || (TestUtils.getProperty(connectionString, "msiClientId") != null
                                     && e.getMessage().toLowerCase()
-                                            .contains(TestResource.getResource("R_loginFailedMI").toLowerCase())),
+                                        .contains(TestResource.getResource("R_loginFailedMI").toLowerCase())),
                     e.getMessage());
             timerEnd = System.currentTimeMillis();
         }
@@ -833,9 +851,9 @@ public void testIncorrectUserName() throws SQLException {
         } catch (Exception e) {
             assertTrue(
                     e.getMessage().contains(TestResource.getResource("R_loginFailed"))
-                            || (TestUtils.getProperty(connectionString, "msiClientId") != null
+                                || (TestUtils.getProperty(connectionString, "msiClientId") != null
                                     && e.getMessage().toLowerCase()
-                                            .contains(TestResource.getResource("R_loginFailedMI").toLowerCase())),
+                                        .contains(TestResource.getResource("R_loginFailedMI").toLowerCase())),
                     e.getMessage());
             timerEnd = System.currentTimeMillis();
         }
@@ -867,8 +885,8 @@ public void testIncorrectPassword() throws SQLException {
             assertTrue(
                     e.getMessage().contains(TestResource.getResource("R_loginFailed"))
                             || (TestUtils.getProperty(connectionString, "msiClientId") != null
-                                    && e.getMessage().toLowerCase()
-                                            .contains(TestResource.getResource("R_loginFailedMI").toLowerCase())),
+                                && e.getMessage().toLowerCase()
+                                    .contains(TestResource.getResource("R_loginFailedMI").toLowerCase())),
                     e.getMessage());
             timerEnd = System.currentTimeMillis();
         }
@@ -1047,8 +1065,8 @@ public void run() {
                 ds.setURL(connectionString);
                 ds.setServerName("invalidServerName" + UUID.randomUUID());
                 ds.setLoginTimeout(30);
-                ds.setConnectRetryCount(3);
-                ds.setConnectRetryInterval(10);
+                ds.setConnectRetryCount(6);
+                ds.setConnectRetryInterval(20);
                 try (Connection con = ds.getConnection()) {} catch (SQLException e) {}
             }
         };

src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java

@@ -172,18 +172,20 @@ public void testDMLoginTimeoutNotApplied() {
         }
     }
 
-    // Test connect retry set to 0 (disabled)
+    /**
+     * Test connect retry set to 0 (disabled)
+     */
     @Test
     public void testConnectRetryDisable() {
         long totalTime = 0;
         long timerStart = System.currentTimeMillis();
         int interval = defaultTimeout; // long interval so we can tell if there was a retry
         long timeout = defaultTimeout * 2; // long loginTimeout to accommodate the long interval
 
-        // non existent server with long loginTimeout, should return fast if no retries at all
+        // non-existent server with long loginTimeout, should return fast if no retries at all
         try (Connection con = PrepUtil.getConnection(
                 "jdbc:sqlserver://" + randomServer + ";transparentNetworkIPResolution=false;loginTimeout=" + timeout
-                        + ";connectRetryCount=0;connectInterval=" + interval)) {
+                        + ";connectRetryCount=0;connectRetryInterval=" + interval)) {
             fail(TestResource.getResource("R_shouldNotConnect"));
         } catch (Exception e) {
             totalTime = System.currentTimeMillis() - timerStart;
@@ -230,7 +232,9 @@ public void testConnectRetryBadServer() {
                 "total time: " + totalTime + " loginTimeout: " + TimeUnit.SECONDS.toMillis(timeout));
     }
 
-    // Test connect retry for database error
+    /**
+     * Test connect retry, with one retry interval, for database error
+     */
     @Test
     public void testConnectRetryServerError() {
         String auth = TestUtils.getProperty(connectionString, "authentication");
@@ -242,10 +246,10 @@ public void testConnectRetryServerError() {
         int interval = defaultTimeout; // long interval so we can tell if there was a retry
         long timeout = defaultTimeout * 2; // long loginTimeout to accommodate the long interval
 
-        // non existent database with interval < loginTimeout this will generate a 4060 transient error and retry 1 time
+        // non-existent database with interval < loginTimeout this will generate a 4060 transient error and retry 2 times
         try (Connection con = PrepUtil.getConnection(
                 TestUtils.addOrOverrideProperty(connectionString, "database", RandomUtil.getIdentifier("database"))
-                        + ";loginTimeout=" + timeout + ";connectRetryCount=" + 1 + ";connectRetryInterval=" + interval
+                        + ";loginTimeout=" + timeout + ";connectRetryCount=" + 2 + ";connectRetryInterval=" + interval
                         + ";transparentNetworkIPResolution=false")) {
             fail(TestResource.getResource("R_shouldNotConnect"));
         } catch (Exception e) {
@@ -261,14 +265,16 @@ public void testConnectRetryServerError() {
                     e.getMessage());
         }
 
-        // 1 retry should be at least 1 interval long but < 2 intervals
+        // 2 retries should be at least 1 interval long but < 2 intervals (no interval between initial attempt and retry 1)
         assertTrue(TimeUnit.SECONDS.toMillis(interval) < totalTime,
                 "interval: " + TimeUnit.SECONDS.toMillis(interval) + " total time: " + totalTime);
         assertTrue(totalTime < TimeUnit.SECONDS.toMillis(2 * interval),
                 "total time: " + totalTime + " 2 * interval: " + TimeUnit.SECONDS.toMillis(interval));
     }
 
-    // Test connect retry for database error using Datasource
+    /**
+     * Test connect retry, with one retry interval, for database error using Datasource
+     */
     @Test
     public void testConnectRetryServerErrorDS() {
         String auth = TestUtils.getProperty(connectionString, "authentication");
@@ -280,10 +286,10 @@ public void testConnectRetryServerErrorDS() {
         int interval = defaultTimeout; // long interval so we can tell if there was a retry
         long loginTimeout = defaultTimeout * 2; // long loginTimeout to accommodate the long interval
 
-        // non existent database with interval < loginTimeout this will generate a 4060 transient error and retry 1 time
+        // non-existent database with interval < loginTimeout this will generate a 4060 transient error and retry 2 times
         SQLServerDataSource ds = new SQLServerDataSource();
         String connectStr = TestUtils.addOrOverrideProperty(connectionString, "database",
-                RandomUtil.getIdentifier("database")) + ";logintimeout=" + loginTimeout + ";connectRetryCount=1"
+                RandomUtil.getIdentifier("database")) + ";loginTimeout=" + loginTimeout + ";connectRetryCount=2"
                 + ";connectRetryInterval=" + interval;
         updateDataSource(connectStr, ds);
 
@@ -301,7 +307,7 @@ public void testConnectRetryServerErrorDS() {
             totalTime = System.currentTimeMillis() - timerStart;
         }
 
-        // 1 retry should be at least 1 interval long but < 2 intervals
+        // 2 retries should be at least 1 interval long but < 2 intervals (no interval between initial attempt and retry 1)
         assertTrue(TimeUnit.SECONDS.toMillis(interval) < totalTime,
                 "interval: " + TimeUnit.SECONDS.toMillis(interval) + " total time: " + totalTime);
         assertTrue(totalTime < TimeUnit.SECONDS.toMillis(2 * interval),

src/test/java/com/microsoft/sqlserver/jdbc/unit/statement/BatchExecutionTest.java

@@ -246,7 +246,6 @@ public void testSqlServerBulkCopyCachingConnectionLevelMultiThreaded() throws Ex
             TimerTask task = new TimerTask() {
                 public void run() {
                     ((HashMap<?, ?>) bulkcopyCache).clear();
-                    fail(TestResource.getResource("R_executionTooLong"));
                 }
             };
             Timer timer = new Timer("Timer");
@@ -348,7 +347,7 @@ public void testValidTimezoneForTimestampBatchInsertWithBulkCopy() throws Except
     public void testValidTimezonesDstTimestampBatchInsertWithBulkCopy() throws Exception {
         Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone("GMT"));
 
-        for (String tzId: TimeZone.getAvailableIDs()) {
+        for (String tzId : TimeZone.getAvailableIDs()) {
             TimeZone.setDefault(TimeZone.getTimeZone(tzId));
 
             long ms = 1696127400000L; // DST