Make the memory store operations serial.

Bug: 128499160
Test: new test in this patch

Change-Id: I6ccdc801e3888a61b22272c8ce9480f45fa26df2
(cherry picked from commit 3074f10b41fd1b622dc698d89e4c9e1bcb5b05cf)
Merged-In: I10b5c5cd85fcb76924ba96c8c379be677774705d

2 files changed, 32 insertions, 18 deletions

diff --git a/packages/NetworkStack/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreService.java b/packages/NetworkStack/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreService.java
index ad2bae89f551..55ab8d4d1376 100644
--- a/packages/NetworkStack/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreService.java
+++ b/packages/NetworkStack/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreService.java
@@ -60,7 +60,6 @@ import java.util.concurrent.Executors;
  */
 public class IpMemoryStoreService extends IIpMemoryStore.Stub {
     private static final String TAG = IpMemoryStoreService.class.getSimpleName();
-    private static final int MAX_CONCURRENT_THREADS = 4;
     private static final int DATABASE_SIZE_THRESHOLD = 10 * 1024 * 1024; //10MB
     private static final int MAX_DROP_RECORD_TIMES = 500;
     private static final int MIN_DELETE_NUM = 5;
@@ -107,23 +106,17 @@ public class IpMemoryStoreService extends IIpMemoryStore.Stub {
             db = null;
         }
         mDb = db;
-        // The work-stealing thread pool executor will spawn threads as needed up to
-        // the max only when there is no free thread available. This generally behaves
-        // exactly like one would expect it intuitively :
-        // - When work arrives, it will spawn a new thread iff there are no available threads
-        // - When there is no work to do it will shutdown threads after a while (the while
-        //   being equal to 2 seconds (not configurable) when max threads are spun up and
-        //   twice as much for every one less thread)
-        // - When all threads are busy the work is enqueued and waits for any worker
-        //   to become available.
-        // Because the stealing pool is made for very heavily parallel execution of
-        // small tasks that spawn others, it creates a queue per thread that in this
-        // case is overhead. However, the three behaviors above make it a superior
-        // choice to cached or fixedThreadPoolExecutor, neither of which can actually
-        // enqueue a task waiting for a thread to be free. This can probably be solved
-        // with judicious subclassing of ThreadPoolExecutor, but that's a lot of dangerous
-        // complexity for little benefit in this case.
-        mExecutor = Executors.newWorkStealingPool(MAX_CONCURRENT_THREADS);
+        // The single thread executor guarantees that all work is executed sequentially on the
+        // same thread, and no two tasks can be active at the same time. This is required to
+        // ensure operations from multiple clients don't interfere with each other (in particular,
+        // operations involving a transaction must not run concurrently with other operations
+        // as the other operations might be taken as part of the transaction). By default, the
+        // single thread executor runs off an unbounded queue.
+        // TODO : investigate replacing this scheme with a scheme where each thread has its own
+        // instance of the database, as it may be faster. It is likely however that IpMemoryStore
+        // operations are mostly IO-bound anyway, and additional contention is unlikely to bring
+        // benefits. Alternatively, a read-write lock might increase throughput.
+        mExecutor = Executors.newSingleThreadExecutor();
         RegularMaintenanceJobService.schedule(mContext, this);
     }
 
diff --git a/packages/NetworkStack/tests/unit/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreServiceTest.java b/packages/NetworkStack/tests/unit/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreServiceTest.java
index c1d6a05fb559..64fe3a6f8e39 100644
--- a/packages/NetworkStack/tests/unit/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreServiceTest.java
+++ b/packages/NetworkStack/tests/unit/src/com/android/server/connectivity/ipmemorystore/IpMemoryStoreServiceTest.java
@@ -732,4 +732,25 @@ public class IpMemoryStoreServiceTest {
                             latch.countDown();
                         })));
     }
+
+    public void testTasksAreSerial() {
+        final long sleepTimeMs = 1000;
+        final long startTime = System.currentTimeMillis();
+        mService.retrieveNetworkAttributes("somekey", onNetworkAttributesRetrieved(
+                (status, key, attr) -> {
+                    assertTrue("Unexpected status : " + status.resultCode, status.isSuccess());
+                    try {
+                        Thread.sleep(sleepTimeMs);
+                    } catch (InterruptedException e) {
+                        fail("InterruptedException");
+                    }
+                }));
+        doLatched("Serial tasks timing out", latch ->
+                mService.retrieveNetworkAttributes("somekey", onNetworkAttributesRetrieved(
+                        (status, key, attr) -> {
+                            assertTrue("Unexpected status : " + status.resultCode,
+                                    status.isSuccess());
+                            assertTrue(System.currentTimeMillis() >= startTime + sleepTimeMs);
+                        })), DEFAULT_TIMEOUT_MS);
+    }
 }