Reintroduce previously removed exit condition in login loop + fix ret…

…ry count (#2212)

* Reintroduced the connected state escape in login loop/pre-increment reconnection attempt number to avoid extra reconnection attempt.

* Added test

* Adjusted timing in test, was undercounting the amount of time taken

* Changed tests

* Fixed imports

* Made loginTimeout a more reasonable 90sec vs 3hours

* Review comments + made timeout longer for case with retry (90 --> 120)

* First test 25sec --> 9sec

* First test 9sec --> 3sec

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

@@ -1859,7 +1859,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 "
@@ -3249,6 +3249,7 @@ private void login(String primary, String primaryInstanceName, int primaryPortNu
                         || (SQLServerException.ERROR_SOCKET_TIMEOUT == driverErrorCode // socket timeout
                                 && (!isDBMirroring || attemptNumber > 0)) // If mirroring, only close after failover has been tried (attempt >= 1)
                         || timerHasExpired(timerExpire)
+                        || (state.equals(State.CONNECTED) && !isDBMirroring)
                 // for non-dbmirroring cases, do not retry after tcp socket connection succeeds
                 ) {
                     // close the connection and throw the error back

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

@@ -442,6 +442,75 @@ public void testConnectionPoolGetTwice() throws SQLException {
         }
     }
 
+    /**
+     * 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() {
+        long timerStart = 0;
+
+        int connectRetryCount = 3;
+        int connectRetryInterval = 1;
+        int longLoginTimeout = loginTimeOutInSeconds * 4; // 120 seconds
+
+        try {
+            SQLServerDataSource ds = new SQLServerDataSource();
+            ds.setURL(connectionString);
+            ds.setLoginTimeout(longLoginTimeout);
+            ds.setConnectRetryCount(connectRetryCount);
+            ds.setConnectRetryInterval(connectRetryInterval);
+            ds.setDatabaseName(RandomUtil.getIdentifier("DataBase"));
+            timerStart = System.currentTimeMillis();
+
+            try (Connection con = ds.getConnection()) {
+                assertTrue(con == null, TestResource.getResource("R_shouldNotConnect"));
+            }
+        } catch (Exception e) {
+            assertTrue(e.getMessage().contains(TestResource.getResource("R_cannotOpenDatabase")), e.getMessage());
+            long totalTime = System.currentTimeMillis() - timerStart;
+            int expectedMinimumTimeInMillis = (connectRetryCount * connectRetryInterval) * 1000; // 3 seconds
+
+            // Minimum time is 0 seconds per attempt and connectRetryInterval * connectRetryCount seconds of interval.
+            // Maximum is unknown, but is needs to be less than longLoginTimeout or else this is an issue.
+            assertTrue(totalTime > expectedMinimumTimeInMillis, TestResource.getResource("R_executionNotLong"));
+            assertTrue(totalTime < (longLoginTimeout * 1000L), TestResource.getResource("R_executionTooLong"));
+        }
+    }
+
+    /**
+     * Tests whether connectRetryCount and connectRetryInterval are properly respected in the login loop. As well, tests
+     * that connection is retried the proper number of times. This is for cases with zero retries.
+     */
+    @Test
+    public void testConnectCountInLoginAndCorrectRetryCountWithZeroRetry() {
+        long timerStart = 0;
+
+        int connectRetryCount = 0;
+        int connectRetryInterval = 60;
+        int longLoginTimeout = loginTimeOutInSeconds * 3; // 90 seconds
+
+        try {
+            SQLServerDataSource ds = new SQLServerDataSource();
+            ds.setURL(connectionString);
+            ds.setLoginTimeout(longLoginTimeout);
+            ds.setConnectRetryCount(connectRetryCount);
+            ds.setConnectRetryInterval(connectRetryInterval);
+            ds.setDatabaseName(RandomUtil.getIdentifier("DataBase"));
+            timerStart = System.currentTimeMillis();
+
+            try (Connection con = ds.getConnection()) {
+                assertTrue(con == null, TestResource.getResource("R_shouldNotConnect"));
+            }
+        } catch (Exception e) {
+            assertTrue(e.getMessage().contains(TestResource.getResource("R_cannotOpenDatabase")), e.getMessage());
+            long totalTime = System.currentTimeMillis() - timerStart;
+
+            // 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"));
+        }
+    }
+
     @Test
     @Tag(Constants.xAzureSQLDW)
     @Tag(Constants.xAzureSQLDB)