Fixes FoundationDB#1659: Add a missing synchronized keyword to Transa…

…ctionalRunner.close This existed ot FDBDatabaseRunnerImpl, but not TransactionalRunner, so closing at the same time as calling `run` can cause a ConcurrentModificationException.
ScottDugas · May 9, 2022 · fc0055c · fc0055c
1 parent d19f7b2
commit fc0055c
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 16 deletions.
diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md
@@ -21,7 +21,7 @@ This release also updates downstream dependency versions. Most notably, the prot
 ### NEXT_RELEASE
 
 * **Bug fix** Fix 1 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
-* **Bug fix** Fix 2 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
+* **Bug fix** Synchronize TransactionalRunner.close [(Issue #1659)](https://github.com/FoundationDB/fdb-record-layer/issues/1659)
 * **Bug fix** Fix 3 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Bug fix** Fix 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Bug fix** Fix 5 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)

diff --git a/...java/com/apple/foundationdb/record/provider/foundationdb/runners/TransactionalRunner.java b/...java/com/apple/foundationdb/record/provider/foundationdb/runners/TransactionalRunner.java
@@ -167,7 +167,7 @@ private synchronized void addContextToClose(@Nonnull FDBRecordContext context) {
     }
 
     @Override
-    public void close() {
+    public synchronized void close() {
         if (closed) {
             return;
         }

diff --git a/.../com/apple/foundationdb/record/provider/foundationdb/runners/TransactionalRunnerTest.java b/.../com/apple/foundationdb/record/provider/foundationdb/runners/TransactionalRunnerTest.java
@@ -282,22 +282,31 @@ private <T extends Exception> void runWithWeakReadSemantics(
     }
 
     @Test
-    void closesContextsSynchronous() {
-        closesContext((runner, contexts, completed) ->
-                // You shouldn't be doing this, but maybe I haven't thought of something similar, but reasonable, where
-                // the executable for `run` does not complete, but the runner is closed
-                CompletableFuture.runAsync(() -> {
-                    runner.run(false, context -> {
-                        contexts.add(context);
-                        new CompletableFuture<Void>().join(); // never joins
-                        return completed.incrementAndGet();
-                    });
-                })
-        );
+    void closesContextsSynchronous() throws InterruptedException {
+        final List<CompletableFuture<Void>> futures = new ArrayList<>();
+        try {
+            closesContext((runner, contexts, completed) ->
+                    // You shouldn't be doing this, but maybe I haven't thought of something similar, but reasonable, where
+                    // the executable for `run` does not complete, but the runner is closed
+                    CompletableFuture.runAsync(() ->
+                            runner.run(false, context -> {
+                                contexts.add(context);
+                                final CompletableFuture<Void> future = new CompletableFuture<>();
+                                futures.add(future);
+                                future.join(); // never joins
+                                return completed.incrementAndGet();
+                            }))
+            );
+        } finally {
+            // cleanup the futures, so that the executor used by runAsync doesn't have a bunch of garbage sitting around
+            // Note: if you remove this, and change the test to @RepeatedTest(100), after 28 repetitions, it fails
+            // consistently.
+            futures.forEach(future -> future.complete(null));
+        }
     }
 
     @Test
-    void closesContexts() {
+    void closesContexts() throws InterruptedException {
         closesContext((runner, contexts, completed) ->
                 runner.runAsync(false, context -> {
                     contexts.add(context);
@@ -308,13 +317,19 @@ void closesContexts() {
         );
     }
 
-    private <T> void closesContext(TriFunction<TransactionalRunner, List<FDBRecordContext>, AtomicInteger, T> run) {
+    private <T> void closesContext(TriFunction<TransactionalRunner, List<FDBRecordContext>, AtomicInteger, T> run)
+            throws InterruptedException {
         List<FDBRecordContext> contexts = new ArrayList<>();
         AtomicInteger completed = new AtomicInteger();
         try (TransactionalRunner runner = defaultTransactionalRunner()) {
             for (int i = 0; i < 10; i++) {
                 run.apply(runner, contexts, completed);
             }
+            // make sure that the contexts have been created
+            while (contexts.size() < 10) {
+                Thread.sleep(100);
+            }
+            assertEquals(0, completed.get());
             for (final FDBRecordContext context : contexts) {
                 assertFalse(context.isClosed());
             }