Skip to content

Commit

Permalink
Fix retries when connectRetryCount is set to 1 or greater (#2513)
Browse files Browse the repository at this point in the history 
* Fix for connectRetryCount > 0

* Cleanup

* Added test to SQLServerConnectionTest + cleanup

* Fix test

* Testing

* ok..

* Grasping at straws

* more debugs, yum

* focus on transient error

* Fix for azure

* Cleanup

* More cleanup

* More cleanup

* Trying to resolve thread interrupt test

* a different try

* add more time to interrupt

* Removed sleep mistakenly

* Added more variants

* Remove unneeded tests

* Remove problem line + add fix for uncaught fail for BatchExecutionTest

* Fail fixes

* Added back the timer to the BatchExecutionTest; adjusted timeout time

* More code cleanup

* Fixed mistake

* Formatting

* Trying to resolve code cov

* Update timeout value

* Remove stray fail

* Revert changes to login retry interval.

* Remove added line.
  • Loading branch information
@Jeffery-Wasty
Jeffery-Wasty authored Nov 7, 2024
1 parent 362d3a4 commit b27b0c1
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 27 deletions.
7 changes: 5 additions & 2 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2031,7 +2031,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 "
Expand Down Expand Up @@ -2064,7 +2064,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));
}
}
}
}
Expand Down
42 changes: 30 additions & 12 deletions src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,17 +456,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();
Expand All @@ -492,6 +501,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"));
}
}
}

Expand Down Expand Up @@ -802,9 +820,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();
}
Expand Down Expand Up @@ -835,9 +853,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();
}
Expand Down Expand Up @@ -869,8 +887,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();
}
Expand Down Expand Up @@ -1049,8 +1067,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) {}
}
};
Expand Down
28 changes: 17 additions & 11 deletions src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand All @@ -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) {
Expand All @@ -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");
Expand All @@ -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);

Expand All @@ -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),
Expand Down
3 changes: 1 addition & 2 deletions src/test/java/com/microsoft/sqlserver/jdbc/unit/statement/BatchExecutionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b27b0c1

Please sign in to comment.