From 1c26cf60efa1b98203af9b21a47e37c8fb1e0e97 Mon Sep 17 00:00:00 2001
From: Arpan Mishra <arpanmishra@google.com>
Date: Tue, 31 Oct 2023 15:36:14 +0530
Subject: [PATCH] fix: prevent illegal negative timeout values into thread
 sleep() method in ITTransactionManagerTest. (#2715)

* Issue - https://togithub.com/googleapis/java-spanner/issues/2634
* Root Cause - In cases where we are receiving AbortedException, the retry delay is set as -1 for the exception. Negative value is not an acceptable input to Thread.sleep() method and hence we are getting IllegalArgumentException. This is resulting in flaky unit test behaviour.
---
 .../spanner/it/ITTransactionManagerTest.java  | 25 +++++++++++++++----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionManagerTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionManagerTest.java
index 736f00ebd69..116d6c24368 100644
--- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionManagerTest.java
+++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionManagerTest.java
@@ -134,7 +134,10 @@ public void simpleInsert() throws InterruptedException {
           assertThat(row.getBoolean(1)).isTrue();
           break;
         } catch (AbortedException e) {
-          Thread.sleep(e.getRetryDelayInMillis());
+          long retryDelayInMillis = e.getRetryDelayInMillis();
+          if (retryDelayInMillis > 0) {
+            Thread.sleep(retryDelayInMillis);
+          }
           txn = manager.resetForRetry();
         }
       }
@@ -158,7 +161,10 @@ public void invalidInsert() throws InterruptedException {
           manager.commit();
           fail("Expected exception");
         } catch (AbortedException e) {
-          Thread.sleep(e.getRetryDelayInMillis());
+          long retryDelayInMillis = e.getRetryDelayInMillis();
+          if (retryDelayInMillis > 0) {
+            Thread.sleep(retryDelayInMillis);
+          }
           txn = manager.resetForRetry();
         } catch (SpannerException e) {
           // expected
@@ -188,7 +194,10 @@ public void rollback() throws InterruptedException {
           manager.rollback();
           break;
         } catch (AbortedException e) {
-          Thread.sleep(e.getRetryDelayInMillis());
+          long retryDelayInMillis = e.getRetryDelayInMillis();
+          if (retryDelayInMillis > 0) {
+            Thread.sleep(retryDelayInMillis);
+          }
           txn = manager.resetForRetry();
         }
       }
@@ -231,7 +240,10 @@ public void abortAndRetry() throws InterruptedException {
           manager1.commit();
           break;
         } catch (AbortedException e) {
-          Thread.sleep(e.getRetryDelayInMillis());
+          long retryDelayInMillis = e.getRetryDelayInMillis();
+          if (retryDelayInMillis > 0) {
+            Thread.sleep(retryDelayInMillis);
+          }
           // It is possible that it was txn2 that aborted.
           // In that case we should just retry without resetting anything.
           if (manager1.getState() == TransactionState.ABORTED) {
@@ -278,7 +290,10 @@ public void testTransactionManagerReturnsCommitStats() throws InterruptedExcepti
           assertEquals(2L, manager.getCommitResponse().getCommitStats().getMutationCount());
           break;
         } catch (AbortedException e) {
-          Thread.sleep(e.getRetryDelayInMillis());
+          long retryDelayInMillis = e.getRetryDelayInMillis();
+          if (retryDelayInMillis > 0) {
+            Thread.sleep(retryDelayInMillis);
+          }
           transaction = manager.resetForRetry();
         }
       }