Fix sql connection recycling bug causing multiple connection reopens

Currently when multiple sql statements that share the same structure
are prepared they are put in a statement cache as preparing them is an
expensive operation. This patch fixes a bug where a non primary
connection would not recycle its opened connection due to it believing
that the pool did not have space caused by a one off error which in turn
caused the connection to be rebuilt every time it was requested.

Bug: 228498581
Test: Verified caching with logs and confirmed savings in traces
Change-Id: Ic7e26db77c23d8ead9565a689800cc48f158c059

core/java/android/database/sqlite/SQLiteConnectionPool.java

@@ -170,8 +170,8 @@ public final class SQLiteConnectionPool implements Closeable {
         // If timeout is set, setup idle connection handler
         // In case of MAX_VALUE - idle connections are never closed
         if (mConfiguration.idleConnectionTimeoutMs != Long.MAX_VALUE) {
-            setupIdleConnectionHandler(Looper.getMainLooper(),
-                    mConfiguration.idleConnectionTimeoutMs);
+            setupIdleConnectionHandler(
+                    Looper.getMainLooper(), mConfiguration.idleConnectionTimeoutMs, null);
         }
     }
 
@@ -425,7 +425,7 @@ public final class SQLiteConnectionPool implements Closeable {
                     mAvailablePrimaryConnection = connection;
                 }
                 wakeConnectionWaitersLocked();
-            } else if (mAvailableNonPrimaryConnections.size() >= mMaxConnectionPoolSize - 1) {
+            } else if (mAvailableNonPrimaryConnections.size() >= mMaxConnectionPoolSize) {
                 closeConnectionAndLogExceptionsLocked(connection);
             } else {
                 if (recycleConnectionLocked(connection, status)) {
@@ -456,6 +456,11 @@ public final class SQLiteConnectionPool implements Closeable {
         return true;
     }
 
+    @VisibleForTesting
+    public boolean hasAnyAvailableNonPrimaryConnection() {
+        return mAvailableNonPrimaryConnections.size() > 0;
+    }
+
     /**
      * Returns true if the session should yield the connection due to
      * contention over available database connections.
@@ -1061,9 +1066,11 @@ public final class SQLiteConnectionPool implements Closeable {
      * Set up the handler based on the provided looper and timeout.
      */
     @VisibleForTesting
-    public void setupIdleConnectionHandler(Looper looper, long timeoutMs) {
+    public void setupIdleConnectionHandler(
+            Looper looper, long timeoutMs, Runnable onAllConnectionsIdle) {
         synchronized (mLock) {
-            mIdleConnectionHandler = new IdleConnectionHandler(looper, timeoutMs);
+            mIdleConnectionHandler =
+                    new IdleConnectionHandler(looper, timeoutMs, onAllConnectionsIdle);
         }
     }
 
@@ -1228,10 +1235,12 @@ public final class SQLiteConnectionPool implements Closeable {
 
     private class IdleConnectionHandler extends Handler {
         private final long mTimeout;
+        private final Runnable mOnAllConnectionsIdle;
 
-        IdleConnectionHandler(Looper looper, long timeout) {
+        IdleConnectionHandler(Looper looper, long timeout, Runnable onAllConnectionsIdle) {
             super(looper);
             mTimeout = timeout;
+            this.mOnAllConnectionsIdle = onAllConnectionsIdle;
         }
 
         @Override
@@ -1247,6 +1256,9 @@ public final class SQLiteConnectionPool implements Closeable {
                                 + " after " + mTimeout);
                     }
                 }
+                if (mOnAllConnectionsIdle != null) {
+                    mOnAllConnectionsIdle.run();
+                }
             }
         }
 

core/tests/coretests/src/android/database/sqlite/SQLiteConnectionPoolTest.java

@@ -33,6 +33,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import java.io.File;
+import java.util.concurrent.CountDownLatch;
 
 /**
  * Tests for {@link SQLiteConnectionPool}
@@ -74,7 +75,7 @@ public class SQLiteConnectionPoolTest {
         Log.i(TAG, "Starting " + thread.getName());
         thread.start();
         SQLiteConnectionPool pool = SQLiteConnectionPool.open(mTestConf);
-        pool.setupIdleConnectionHandler(thread.getLooper(), 100);
+        pool.setupIdleConnectionHandler(thread.getLooper(), 100, null);
         SQLiteConnection c1 = pool.acquireConnection("pragma user_version", 0, null);
         assertEquals("First connection should be returned", 0, c1.getConnectionId());
         pool.releaseConnection(c1);
@@ -89,4 +90,31 @@ public class SQLiteConnectionPoolTest {
         pool.close();
         thread.quit();
     }
+
+    @Test
+    public void testNonprimaryConnectionPoolRecycling() throws InterruptedException {
+        HandlerThread thread = new HandlerThread("test-close-idle-connections-thread");
+        thread.start();
+        SQLiteConnectionPool pool = SQLiteConnectionPool.open(mTestConf);
+        CountDownLatch latch = new CountDownLatch(1);
+        Runnable onIdleConnectionTimeout = () -> latch.countDown();
+        pool.setupIdleConnectionHandler(thread.getLooper(), 1, onIdleConnectionTimeout);
+
+        assertTrue("When the pool was just opened there should only be a primary connection.",
+                !pool.hasAnyAvailableNonPrimaryConnection());
+        SQLiteConnection connection = pool.acquireConnection("pragma user_version", 0, null);
+        pool.releaseConnection(connection);
+        assertTrue("First time acquire should will return the primary connection.",
+                !pool.hasAnyAvailableNonPrimaryConnection());
+
+        // Wait for primary connection to time out
+        latch.await();
+
+        // Now that the primary is closed, acquiring again should open a non primary connection
+        connection = pool.acquireConnection("pragma user_version", 0, null);
+        pool.releaseConnection(connection);
+        assertTrue("There should be an available non primary connection in the pool.",
+                pool.hasAnyAvailableNonPrimaryConnection());
+        pool.close();
+    }
 }