fix(6.4.4-sp): sessions were not always removed from checkedOutSessions (#1445)

* fix: sessions were not always removed from checkedOutSessions

Sessions were added to a set of checked out sessions when one was checked out from
the session pool. When the session was released back to the pool, the session would
normally be removed from the set of checked out sessions. The latter would not always
happen if the application that checked out the session did not use the session for
any reads or writes.

chore: fix maven version to 3.8.1

chore: fix maven version for windows

* test: skip slow tests on stable branch

* test: try to add compile workflow to see if build succeeds

* test: add compile job instead of file

Author: olavloite

.github/workflows/ci.yaml

@@ -14,6 +14,9 @@ jobs:
           - 11
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: ${{matrix.java}}
@@ -25,6 +28,9 @@ jobs:
     runs-on: windows-latest
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: 8
@@ -41,6 +47,9 @@ jobs:
           - 11
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: ${{matrix.java}}
@@ -50,6 +59,9 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: 8
@@ -61,10 +73,32 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: 8
       - run: java -version
       - run: .kokoro/build.sh
         env:
           JOB_TYPE: clirr
+  compile:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        java:
+          - 8
+          - 11
+    steps:
+      - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
+      - uses: actions/setup-java@v1
+        with:
+          java-version: ${{matrix.java}}
+      - run: java -version
+      - run: mvn package
+        env:
+          JOB_TYPE: test

.github/workflows/integration-tests-against-emulator.yaml

@@ -15,6 +15,9 @@ jobs:
           - '9020:9020'
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: 8

.github/workflows/samples.yaml

@@ -6,6 +6,9 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2
+      - uses: stCarolas/setup-maven@v4
+        with:
+          maven-version: 3.8.1
       - uses: actions/setup-java@v1
         with:
           java-version: 8

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

@@ -1240,25 +1240,27 @@ public void prepareReadWriteTransaction() {
 
     @Override
     public void close() {
-      synchronized (lock) {
-        leakedException = null;
-        checkedOutSessions.remove(this);
-      }
-      PooledSession delegate = getOrNull();
-      if (delegate != null) {
-        delegate.close();
+      try {
+        asyncClose().get();
+      } catch (InterruptedException e) {
+        throw SpannerExceptionFactory.propagateInterrupt(e);
+      } catch (ExecutionException e) {
+        throw SpannerExceptionFactory.asSpannerException(e.getCause());
       }
     }
 
     @Override
     public ApiFuture<Empty> asyncClose() {
-      synchronized (lock) {
-        leakedException = null;
-        checkedOutSessions.remove(this);
-      }
-      PooledSession delegate = getOrNull();
-      if (delegate != null) {
-        return delegate.asyncClose();
+      try {
+        PooledSession delegate = getOrNull();
+        if (delegate != null) {
+          return delegate.asyncClose();
+        }
+      } finally {
+        synchronized (lock) {
+          leakedException = null;
+          checkedOutSessions.remove(this);
+        }
       }
       return ApiFutures.immediateFuture(Empty.getDefaultInstance());
     }
@@ -1767,7 +1769,8 @@ private enum Position {
   private final Set<PooledSession> allSessions = new HashSet<>();
 
   @GuardedBy("lock")
-  private final Set<PooledSessionFuture> checkedOutSessions = new HashSet<>();
+  @VisibleForTesting
+  final Set<PooledSessionFuture> checkedOutSessions = new HashSet<>();
 
   private final SessionConsumer sessionConsumer = new SessionConsumerImpl();
 

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

@@ -69,6 +69,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -534,13 +535,18 @@ public void testAsyncTransactionManagerCommitWithTag() {
 
   @Test
   public void singleUse() {
-    DatabaseClient client =
-        spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    DatabaseClientImpl client =
+        (DatabaseClientImpl)
+            spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    Set<PooledSessionFuture> checkedOut = client.pool.checkedOutSessions;
+    assertThat(checkedOut).isEmpty();
     try (ResultSet rs = client.singleUse().executeQuery(SELECT1)) {
       assertThat(rs.next()).isTrue();
+      assertThat(checkedOut).hasSize(1);
       assertThat(rs.getLong(0)).isEqualTo(1L);
       assertThat(rs.next()).isFalse();
     }
+    assertThat(checkedOut).isEmpty();
   }
 
   @Test
@@ -2076,4 +2082,71 @@ public void testAsyncTransactionManagerCommitWithPriority() {
     assertNotNull(request.getRequestOptions());
     assertEquals(Priority.PRIORITY_HIGH, request.getRequestOptions().getPriority());
   }
+
+  @Test
+  public void singleUseNoAction_ClearsCheckedOutSession() {
+    DatabaseClientImpl client =
+        (DatabaseClientImpl)
+            spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    Set<PooledSessionFuture> checkedOut = client.pool.checkedOutSessions;
+    assertThat(checkedOut).isEmpty();
+
+    // Getting a single use read-only transaction and not using it should not cause any sessions
+    // to be stuck in the map of checked out sessions.
+    client.singleUse().close();
+
+    assertThat(checkedOut).isEmpty();
+  }
+
+  @Test
+  public void singleUseReadOnlyTransactionNoAction_ClearsCheckedOutSession() {
+    DatabaseClientImpl client =
+        (DatabaseClientImpl)
+            spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    Set<PooledSessionFuture> checkedOut = client.pool.checkedOutSessions;
+    assertThat(checkedOut).isEmpty();
+
+    client.singleUseReadOnlyTransaction().close();
+
+    assertThat(checkedOut).isEmpty();
+  }
+
+  @Test
+  public void readWriteTransactionNoAction_ClearsCheckedOutSession() {
+    DatabaseClientImpl client =
+        (DatabaseClientImpl)
+            spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    Set<PooledSessionFuture> checkedOut = client.pool.checkedOutSessions;
+    assertThat(checkedOut).isEmpty();
+
+    client.readWriteTransaction();
+
+    assertThat(checkedOut).isEmpty();
+  }
+
+  @Test
+  public void readOnlyTransactionNoAction_ClearsCheckedOutSession() {
+    DatabaseClientImpl client =
+        (DatabaseClientImpl)
+            spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    Set<PooledSessionFuture> checkedOut = client.pool.checkedOutSessions;
+    assertThat(checkedOut).isEmpty();
+
+    client.readOnlyTransaction().close();
+
+    assertThat(checkedOut).isEmpty();
+  }
+
+  @Test
+  public void transactionManagerNoAction_ClearsCheckedOutSession() {
+    DatabaseClientImpl client =
+        (DatabaseClientImpl)
+            spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+    Set<PooledSessionFuture> checkedOut = client.pool.checkedOutSessions;
+    assertThat(checkedOut).isEmpty();
+
+    client.transactionManager().close();
+
+    assertThat(checkedOut).isEmpty();
+  }
 }

google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBackupTest.java

@@ -76,6 +76,7 @@
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.junit.runner.RunWith;
@@ -206,6 +207,7 @@ private void waitForDbOperations(String backupId) throws InterruptedException {
   }
 
   @Test
+  @Ignore("Do not run slow test in stable branch")
   public void testBackups() throws InterruptedException, ExecutionException {
     // Create two test databases in parallel.
     final String db1Id = testHelper.getUniqueDatabaseId() + "_db1";
@@ -383,6 +385,7 @@ public void testBackups() throws InterruptedException, ExecutionException {
   }
 
   @Test(expected = SpannerException.class)
+  @Ignore("Do not run slow test in stable branch")
   public void backupCreationWithVersionTimeTooFarInThePastFails() throws Exception {
     final Database testDatabase = testHelper.createTestDatabase();
     final DatabaseId databaseId = testDatabase.getId();
@@ -402,6 +405,7 @@ public void backupCreationWithVersionTimeTooFarInThePastFails() throws Exception
   }
 
   @Test(expected = SpannerException.class)
+  @Ignore("Do not run slow test in stable branch")
   public void backupCreationWithVersionTimeInTheFutureFails() throws Exception {
     final Database testDatabase = testHelper.createTestDatabase();
     final DatabaseId databaseId = testDatabase.getId();