From fa5e624c5009fc141b1462ed4fe976d767877051 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 4 Jul 2019 11:19:51 -0400 Subject: [PATCH 01/11] Deregister firestore when shutdown + app.delete calls shutdown --- .../firebase/firestore/FirestoreTest.java | 40 +++++++++++++++++++ .../testutil/IntegrationTestUtil.java | 5 +++ .../firebase/firestore/FirebaseFirestore.java | 21 ++++++++-- .../firestore/FirestoreMultiDbComponent.java | 19 +++++++-- .../errorprone/ComponentsAppGetCheck.java | 19 ++++++++- 5 files changed, 96 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 83655041b6d..837d480fad1 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -20,6 +20,7 @@ import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollectionWithDocs; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testDocument; +import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testFirebaseApp; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testFirestore; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitFor; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitForException; @@ -30,6 +31,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; @@ -38,6 +40,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.firebase.FirebaseApp; import com.google.firebase.Timestamp; import com.google.firebase.firestore.FirebaseFirestoreException.Code; import com.google.firebase.firestore.Query.Direction; @@ -1023,4 +1026,41 @@ public void testClearPersistenceWhileRunningFails() { FirebaseFirestoreException firestoreException = (FirebaseFirestoreException) e; assertEquals(Code.FAILED_PRECONDITION, firestoreException.getCode()); } + + @Test + public void testRestartFirestoreLeadsToNewInstance() { + FirebaseApp app = testFirebaseApp(); + FirebaseFirestore instance = FirebaseFirestore.getInstance(app); + FirebaseFirestore sameInstance = FirebaseFirestore.getInstance(app); + + assertSame(instance, sameInstance); + waitFor(instance.document("abc/123").set(Collections.singletonMap("field", 100L))); + + instance.shutdown(); + FirebaseFirestore newInstance = FirebaseFirestore.getInstance(app); + + // Verify new instance works. + DocumentSnapshot doc = waitFor(newInstance.document("abc/123").get()); + assertEquals(doc.get("field"), 100L); + waitFor(newInstance.document("abc/123").delete()); + + // Verify it is different instance. + assertNotSame(instance, newInstance); + } + + /* + @Test + public void testAppDeleteLeadsToFirestoreShutdown() { + FirebaseApp app = testFirebaseApp(); + FirebaseFirestore instance = FirebaseFirestore.getInstance(app); + instance.document("abc/123").set(Collections.singletonMap("Field", 100)); + + app.delete(); + + try { + instance.document("abc/123").get(); + } catch (Exception e) { + + } + }*/ } diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 5f383d4b93b..3f476ba07fb 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -25,6 +25,7 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.FirebaseApp; import com.google.firebase.firestore.AccessHelper; import com.google.firebase.firestore.BuildConfig; import com.google.firebase.firestore.CollectionReference; @@ -145,6 +146,10 @@ protected String checkAuthority(String authority) { return settings.build(); } + public static FirebaseApp testFirebaseApp() { + return FirebaseApp.initializeApp(ApplicationProvider.getApplicationContext()); + } + /** Initializes a new Firestore instance that uses the default project. */ public static FirebaseFirestore testFirestore() { return testFirestore(newTestSettings()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 1d8fb8c625c..27b2ab601dc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -117,7 +117,13 @@ static FirebaseFirestore newInstance( // so there is no need to include it in the persistence key. String persistenceKey = app.getName(); - return new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app); + FirebaseFirestore firestore = + new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app); + app.addLifecycleEventListener( + /* onDeleted */ (firebaseAppName, options) -> { + firestore.shutdown(/* fromAppDeletion */ true); + }); + return firestore; } @VisibleForTesting @@ -329,13 +335,22 @@ public Task runBatch(@NonNull WriteBatch.Function batchFunction) { return batch.commit(); } - @VisibleForTesting - Task shutdown() { + Task shutdown(boolean fromAppDeletion) { + if (!fromAppDeletion && this.getApp() != null) { + FirestoreMultiDbComponent component = this.getApp().get(FirestoreMultiDbComponent.class); + component.remove(this.databaseId.getDatabaseId()); + } // The client must be initialized to ensure that all subsequent API usage throws an exception. this.ensureClientConfigured(); return client.shutdown(); } + @VisibleForTesting + // TODO: Make this public + Task shutdown() { + return shutdown(/* fromAppDeletion */ false); + } + @VisibleForTesting AsyncQueue getAsyncQueue() { return asyncQueue; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java index 3d80efe980b..9dece161ca5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java @@ -45,12 +45,23 @@ class FirestoreMultiDbComponent { /** Provides instances of Firestore for given database names. */ @NonNull - synchronized FirebaseFirestore get(@NonNull String databaseName) { - FirebaseFirestore firestore = instances.get(databaseName); + synchronized FirebaseFirestore get(@NonNull String databaseId) { + FirebaseFirestore firestore = instances.get(databaseId); if (firestore == null) { - firestore = FirebaseFirestore.newInstance(context, app, authProvider, databaseName); - instances.put(databaseName, firestore); + firestore = FirebaseFirestore.newInstance(context, app, authProvider, databaseId); + instances.put(databaseId, firestore); } return firestore; } + + /** + * Remove the instance of a given database name from this component, such that if {@link + * FirestoreMultiDbComponent#get(String)} is called again with the same name, a new instance of + * {@link FirebaseFirestore} is created. + * + *

It is a no-op if there is no instance associated with the given database name. + */ + synchronized void remove(@NonNull String databaseId) { + instances.remove(databaseId); + } } diff --git a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java index b42c5dbfa48..57e849ef510 100644 --- a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java +++ b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.isStatic; +import static com.google.errorprone.matchers.Matchers.isVoidType; import static com.google.errorprone.matchers.Matchers.methodHasVisibility; import static com.google.errorprone.matchers.Matchers.methodInvocation; import static com.google.errorprone.matchers.Matchers.methodIsNamed; @@ -87,6 +88,22 @@ public class ComponentsAppGetCheck extends BugChecker isSupertypeOf( state -> ASTHelpers.getType(state.findEnclosing(ClassTree.class)))))); + /** + * This matches methods of the forms: + * + * 

{@code
+   * class Foo {
+   *     void shutdown();
+   * }
+   *
+ */ + private static final Matcher WITHIN_SHUTDOWN = + enclosingMethod( + allOf( + methodIsNamed("shutdown"))); + //methodReturns(isVoidType()))); + + /** * This matches methods of the forms: * @@ -114,7 +131,7 @@ public class ComponentsAppGetCheck extends BugChecker enclosingClass(hasAnnotation("org.junit.runner.RunWith")); private static final Matcher ALLOWED_USAGES = - anyOf(WITHIN_GET_INSTANCE, WITHIN_GET_INSTANCE_IMPL, WITHIN_JUNIT_TEST); + anyOf(WITHIN_GET_INSTANCE, WITHIN_GET_INSTANCE_IMPL, WITHIN_SHUTDOWN, WITHIN_JUNIT_TEST); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { From 9ba6ff3556899d20fdf4c00b38480bffa0084a0f Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 4 Jul 2019 17:11:58 -0400 Subject: [PATCH 02/11] synchronized scheduling + soft shutdown --- .../firebase/firestore/FirestoreTest.java | 19 +- .../firebase/firestore/FirebaseFirestore.java | 2 +- .../firestore/core/FirestoreClient.java | 23 +- .../firestore/remote/GrpcCallProvider.java | 2 +- .../firebase/firestore/util/AsyncQueue.java | 259 +++++++++++++----- .../firestore/util/AsyncQueueTest.java | 16 ++ 6 files changed, 235 insertions(+), 86 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 837d480fad1..eda39ef1b53 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1048,19 +1048,24 @@ public void testRestartFirestoreLeadsToNewInstance() { assertNotSame(instance, newInstance); } - /* @Test public void testAppDeleteLeadsToFirestoreShutdown() { FirebaseApp app = testFirebaseApp(); FirebaseFirestore instance = FirebaseFirestore.getInstance(app); - instance.document("abc/123").set(Collections.singletonMap("Field", 100)); + waitFor(instance.document("abc/123").set(Collections.singletonMap("Field", 100))); app.delete(); - try { - instance.document("abc/123").get(); - } catch (Exception e) { + assertTrue(instance.getClient().isShutdown()); + } - } - }*/ + @Test(expected = IllegalStateException.class) + public void testNewOperationThrowsAfterFirestoreShutdown() { + FirebaseFirestore instance = testFirestore(); + waitFor(instance.document("abc/123").set(Collections.singletonMap("Field", 100))); + + instance.shutdown(); + + waitForException(instance.document("abc/123").get()); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 27b2ab601dc..f3a6fc6b5d2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -411,7 +411,7 @@ public static void setLoggingEnabled(boolean loggingEnabled) { @PublicApi public Task clearPersistence() { final TaskCompletionSource source = new TaskCompletionSource<>(); - asyncQueue.enqueueAndForget( + asyncQueue.enqueueAndForgetEvenAfterShutdown( () -> { try { if (client != null && !client.isShutdown()) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index a2ddd64a8fc..250cb24c774 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -75,7 +75,6 @@ public final class FirestoreClient implements RemoteStore.RemoteStoreCallback { private RemoteStore remoteStore; private SyncEngine syncEngine; private EventManager eventManager; - private volatile boolean clientShutdown = false; // LRU-related @Nullable private LruGarbageCollector.Scheduler lruScheduler; @@ -138,21 +137,21 @@ public Task enableNetwork() { /** Shuts down this client, cancels all writes / listeners, and releases all resources. */ public Task shutdown() { credentialsProvider.removeChangeListener(); - return asyncQueue.enqueue( + return asyncQueue.enqueueAndInitiateShutdown( () -> { - if (!this.clientShutdown) { - remoteStore.shutdown(); - persistence.shutdown(); - if (lruScheduler != null) { - lruScheduler.stop(); - } - this.clientShutdown = true; + remoteStore.shutdown(); + persistence.shutdown(); + if (lruScheduler != null) { + lruScheduler.stop(); } }); } + /** Has this client been shutdown. */ public boolean isShutdown() { - return this.clientShutdown; + // Technically, the asyncQueue is still running, but only accepting tasks related to shutdown + // or supposed to be run after shutdown. It is effectively shut down to the eyes of users. + return this.asyncQueue.isShuttingDown(); } /** Starts listening to a query. */ @@ -272,8 +271,8 @@ private void initialize(Context context, User user, boolean usePersistence, long } private void verifyNotShutdown() { - if (this.clientShutdown) { - throw new IllegalArgumentException("The client has already been shutdown"); + if (this.isShutdown()) { + throw new IllegalStateException("The client has already been shutdown"); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java index f1803dd72de..51505926a60 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java @@ -138,7 +138,7 @@ Task> createClientCall( /** Shuts down the gRPC channel and the internal worker queue. */ void shutdown() { channelTask.addOnCompleteListener( - asyncQueue.getExecutor(), + asyncQueue.getExecutorForShutdown(), task -> { ManagedChannel channel = task.getResult(); channel.shutdown(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java index 039af4b490f..8c29f1df292 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java @@ -176,97 +176,180 @@ public static Task callTask(Executor executor, Callable + *
  • 1. Synchronized task scheduling. This is different from function 3, which is about task + * execution in a single thread. + *
  • 2. Ability to do soft-shutdown: only critical tasks related to shutting Firestore SDK + * down can be executed once the shutdown process initiated. + *
  • 3. Single threaded execution service, no concurrent execution among the `Runnable`s + * scheduled in this Executor. + * */ - private final Thread thread; + private class SynchronizedShutdownAwareExecutor implements Executor { + /** + * The single threaded executor that is backing this Executor. This is also the executor used + * when some tasks explicitly request to run after shutdown has been initiated. + */ + private final ScheduledThreadPoolExecutor internalExecutor; - /** The single threaded executor that is backing this AsyncQueue */ - private final ScheduledThreadPoolExecutor executor; + /** Whether the shutdown process has initiated, once it is started, it is not revertable. */ + private boolean isShuttingDown; - // Tasks scheduled to be queued in the future. Tasks are automatically removed after they are run - // or canceled. - // NOTE: We disallow duplicates currently, so this could be a Set<> which might have better - // theoretical removal speed, except this list will always be small so ArrayList is fine. - private final ArrayList delayedTasks; + /** + * The single thread that will be used by the executor. This is created early and managed + * directly so that it's possible later to make assertions about executing on the correct + * thread. + */ + private final Thread thread; + + /** A ThreadFactory for a single, pre-created thread. */ + private class DelayedStartFactory implements Runnable, ThreadFactory { + private final CountDownLatch latch = new CountDownLatch(1); + private Runnable delegate; + + @Override + public void run() { + try { + latch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + delegate.run(); + } + + @Override + public Thread newThread(@NonNull Runnable runnable) { + hardAssert(delegate == null, "Only one thread may be created in an AsyncQueue."); + delegate = runnable; + latch.countDown(); + return thread; + } + } + + SynchronizedShutdownAwareExecutor() { + DelayedStartFactory threadFactory = new DelayedStartFactory(); + + thread = Executors.defaultThreadFactory().newThread(threadFactory); + thread.setName("FirestoreWorker"); + thread.setDaemon(true); + thread.setUncaughtExceptionHandler((crashingThread, throwable) -> panic(throwable)); + + internalExecutor = + new ScheduledThreadPoolExecutor(1, threadFactory) { + @Override + protected void afterExecute(Runnable r, Throwable t) { + super.afterExecute(r, t); + if (t == null && r instanceof Future) { + Future future = (Future) r; + try { + // Not all Futures will be done, e.g. when used with scheduledAtFixedRate + if (future.isDone()) { + future.get(); + } + } catch (CancellationException ce) { + // Cancellation exceptions are okay, we expect them to happen sometimes + } catch (ExecutionException ee) { + t = ee.getCause(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + if (t != null) { + panic(t); + } + } + }; + + // Core threads don't time out, this only takes effect when we drop the number of required + // core threads + internalExecutor.setKeepAliveTime(3, TimeUnit.SECONDS); - /** A ThreadFactory for a single, pre-created thread. */ - private class DelayedStartFactory implements Runnable, ThreadFactory { - private final CountDownLatch latch = new CountDownLatch(1); - private Runnable delegate; + isShuttingDown = false; + } + + /** Synchronized access to isShuttingDown */ + private synchronized boolean isShuttingDown() { + return isShuttingDown; + } + /** + * Check if shutdown is initiated before scheduling. If it is initiated, the command will not be + * executed. + */ @Override - public void run() { - try { - latch.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); + public synchronized void execute(Runnable command) { + if (!isShuttingDown) { + internalExecutor.execute(command); } - delegate.run(); } - @Override - public Thread newThread(@NonNull Runnable runnable) { - hardAssert(delegate == null, "Only one thread may be created in an AsyncQueue."); - delegate = runnable; - latch.countDown(); - return thread; + /** + * Initiate the shutdown process. Once called, the only possible way to run `Runnable`s are by + * holding the `internalExecutor` reference. + */ + private synchronized void initiateShutdown() { + if (!isShuttingDown) { + isShuttingDown = true; + } + } + + /** + * Wraps {@link ScheduledThreadPoolExecutor#schedule(Runnable, long, TimeUnit)} and provides + * shutdown state check: the command will not be scheduled if the shutdown has been initiated. + */ + private synchronized ScheduledFuture schedule(Runnable command, long delay, TimeUnit unit) { + if (!isShuttingDown) { + return internalExecutor.schedule(command, delay, unit); + } + return null; + } + + /** Wraps around {@link ScheduledThreadPoolExecutor#shutdownNow()}. */ + private void shutdownNow() { + internalExecutor.shutdownNow(); + } + + /** Wraps around {@link ScheduledThreadPoolExecutor#setCorePoolSize(int)}. */ + private void setCorePoolSize(int size) { + internalExecutor.setCorePoolSize(size); } } + /** The executor backing this AsyncQueue. */ + private final SynchronizedShutdownAwareExecutor executor; + // Tasks scheduled to be queued in the future. Tasks are automatically removed after they are run + // or canceled. + // NOTE: We disallow duplicates currently, so this could be a Set<> which might have better + // theoretical removal speed, except this list will always be small so ArrayList is fine. + private final ArrayList delayedTasks; + public AsyncQueue() { delayedTasks = new ArrayList<>(); - - DelayedStartFactory threadFactory = new DelayedStartFactory(); - - thread = Executors.defaultThreadFactory().newThread(threadFactory); - thread.setName("FirestoreWorker"); - thread.setDaemon(true); - thread.setUncaughtExceptionHandler((crashingThread, throwable) -> panic(throwable)); - - executor = - new ScheduledThreadPoolExecutor(1, threadFactory) { - @Override - protected void afterExecute(Runnable r, Throwable t) { - super.afterExecute(r, t); - if (t == null && r instanceof Future) { - Future future = (Future) r; - try { - // Not all Futures will be done, e.g. when used with scheduledAtFixedRate - if (future.isDone()) { - future.get(); - } - } catch (CancellationException ce) { - // Cancellation exceptions are okay, we expect them to happen sometimes - } catch (ExecutionException ee) { - t = ee.getCause(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } - if (t != null) { - panic(t); - } - } - }; - - // Core threads don't time out, this only takes effect when we drop the number of required - // core threads - executor.setKeepAliveTime(3, TimeUnit.SECONDS); + executor = new SynchronizedShutdownAwareExecutor(); } public Executor getExecutor() { return executor; } + /** + * For tasks that need to run after shutdown has been initiated. The tasks should themselves be + * shutdown related. + */ + public Executor getExecutorForShutdown() { + return executor.internalExecutor; + } + /** Verifies that the current thread is the managed AsyncQueue thread. */ public void verifyIsCurrentThread() { Thread current = Thread.currentThread(); - if (thread != current) { + if (executor.thread != current) { throw fail( "We are running on the wrong thread. Expected to be on the AsyncQueue " + "thread %s/%d but was %s/%d", - thread.getName(), thread.getId(), current.getName(), current.getId()); + executor.thread.getName(), executor.thread.getId(), current.getName(), current.getId()); } } @@ -279,6 +362,10 @@ public void verifyIsCurrentThread() { */ @CheckReturnValue public Task enqueue(Callable task) { + return enqueueImpl(task, executor); + } + + private Task enqueueImpl(Callable task, Executor executor) { final TaskCompletionSource completionSource = new TaskCompletionSource<>(); try { executor.execute( @@ -313,6 +400,48 @@ public Task enqueue(Runnable task) { }); } + /** + * Queue a Runnable and immediately mark the initiation of shutdown process. Tasks queued after + * this method is called are not run unless they explicitly request via {@link + * AsyncQueue#enqueueAndForgetEvenAfterShutdown(Runnable)} or {@link + * AsyncQueue#getExecutorForShutdown()}. + */ + public Task enqueueAndInitiateShutdown(Runnable task) { + synchronized (executor) { + if (executor.isShuttingDown()) { + TaskCompletionSource source = new TaskCompletionSource<>(); + source.setResult(null); + return source.getTask(); + } + Task t = + enqueue( + () -> { + task.run(); + return null; + }); + executor.initiateShutdown(); + return t; + } + } + + /** + * Queue and run this Runnable task immediately after every other already queued task, regardless + * if shutdown has been initiated. + */ + public void enqueueAndForgetEvenAfterShutdown(Runnable task) { + enqueueImpl( + () -> { + task.run(); + return null; + }, + executor.internalExecutor); + } + + /** Has the shutdown process been initiated. */ + public boolean isShuttingDown() { + return executor.isShuttingDown(); + } + /** * Queue and run this Runnable task immediately after every other already queued task. Unlike * enqueue(), returns void instead of a Task Object for use when we have no need to "wait" on the diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java index ae14bfc155d..396445b2910 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java @@ -123,4 +123,20 @@ public void canManuallyDrainSpecificDelayedTasksForTesting() throws Exception { queue.runDelayedTasksUntil(TIMER_ID_3); assertEquals(Arrays.asList(1, 2, 3, 4), completedSteps); } + + @Test + public void tasksAreScheduledWithRespectToShutdown() { + expectedSteps = Arrays.asList(1, 2, 4, 6); + queue.enqueueAndForget(runnableForStep(1)); + + // From this point on, `normal` tasks are not scheduled. Only those who explicitly request to + // run after shutdown initiated will run. + queue.enqueueAndInitiateShutdown(runnableForStep(2)); + + queue.enqueueAndForget(runnableForStep(3)); + queue.enqueueAndForgetEvenAfterShutdown(runnableForStep(4)); + + queue.getExecutor().execute(runnableForStep(5)); + queue.getExecutorForShutdown().execute(runnableForStep(6)); + } } From 835ddc882099978c8267c3395647627180518c06 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Mon, 8 Jul 2019 15:58:16 -0400 Subject: [PATCH 03/11] Add comments and more tests --- .../firebase/firestore/FirestoreTest.java | 18 ++++++++++++--- .../firebase/firestore/FirebaseFirestore.java | 23 ++++++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index eda39ef1b53..81f50ffaaaa 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1059,13 +1059,25 @@ public void testAppDeleteLeadsToFirestoreShutdown() { assertTrue(instance.getClient().isShutdown()); } - @Test(expected = IllegalStateException.class) + @Test public void testNewOperationThrowsAfterFirestoreShutdown() { FirebaseFirestore instance = testFirestore(); - waitFor(instance.document("abc/123").set(Collections.singletonMap("Field", 100))); + final DocumentReference reference = instance.document("abc/123"); + DocumentReference ref = reference; + waitFor(reference.set(Collections.singletonMap("Field", 100))); instance.shutdown(); - waitForException(instance.document("abc/123").get()); + final String expectedMessage = "The client has already been shutdown"; + expectError(() -> waitFor(reference.get()), expectedMessage); + expectError(() -> waitFor(reference.update("Field", 1)), expectedMessage); + expectError( + () -> waitFor(reference.set(Collections.singletonMap("Field", 1))), expectedMessage); + expectError( + () -> waitFor(instance.runBatch((batch) -> batch.update(reference, "Field", 1))), + expectedMessage); + expectError( + () -> waitFor(instance.runTransaction(transaction -> transaction.get(reference))), + expectedMessage); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index f3a6fc6b5d2..0455d3856f1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -345,8 +345,29 @@ Task shutdown(boolean fromAppDeletion) { return client.shutdown(); } + /** + * Shuts down this Firestore SDK instance. + * + *

    IMPORTANT: If your application is designed to run with Firestore the entire life cycle, it + * is not recommended to call `shutdown` explicitly, the shutting down happens naturally when the + * application itself is shut down. + * + *

    For applications designed to run without Firestore from some point on (typically to save + * resources as much as possible), you may call this method. + * + *

    Once `shutdown` is called, trying to issue new reads/writes through this instance will + * result in {@link IllegalStateException}. Already requested reads/writes might stop resolving. + * Be sure to handle this well in your application if proceeding with Firestore shutting down is + * desired in your case. + * + *

    Note that {@link #clearPersistence()} is an exception, it is designed to run when the + * Firestore SDK instance is shutdown. + * + *

    To re-start a Firestore SDK instance, simply create a new instance via {@link + * #getInstance()} or {@link #getInstance(FirebaseApp)}. + */ @VisibleForTesting - // TODO: Make this public + // TODO: Make this public and remove @VisibleForTesting Task shutdown() { return shutdown(/* fromAppDeletion */ false); } From e7e3ea18155635b56184af90765f97bfd872d58a Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 11 Jul 2019 10:50:22 -0400 Subject: [PATCH 04/11] addressing comments #1 --- .../firebase/firestore/FirestoreTest.java | 20 +++++++++-- .../firebase/firestore/FirebaseFirestore.java | 35 +++++++++---------- .../firestore/FirestoreMultiDbComponent.java | 4 +-- .../firestore/core/FirestoreClient.java | 2 +- .../firebase/firestore/util/AsyncQueue.java | 4 +-- 5 files changed, 40 insertions(+), 25 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 81f50ffaaaa..71a50100837 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1062,8 +1062,7 @@ public void testAppDeleteLeadsToFirestoreShutdown() { @Test public void testNewOperationThrowsAfterFirestoreShutdown() { FirebaseFirestore instance = testFirestore(); - final DocumentReference reference = instance.document("abc/123"); - DocumentReference ref = reference; + DocumentReference reference = instance.document("abc/123"); waitFor(reference.set(Collections.singletonMap("Field", 100))); instance.shutdown(); @@ -1080,4 +1079,21 @@ public void testNewOperationThrowsAfterFirestoreShutdown() { () -> waitFor(instance.runTransaction(transaction -> transaction.get(reference))), expectedMessage); } + + @Test + public void testShutdownCalledMultipleTimes() { + FirebaseFirestore instance = testFirestore(); + DocumentReference reference = instance.document("abc/123"); + waitFor(reference.set(Collections.singletonMap("Field", 100))); + + instance.shutdown(); + + final String expectedMessage = "The client has already been shutdown"; + expectError(() -> waitFor(reference.get()), expectedMessage); + + // Calling a second time should go through and change nothing. + instance.shutdown(); + + expectError(() -> waitFor(reference.get()), expectedMessage); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 0455d3856f1..a075c2ea966 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -120,8 +120,8 @@ static FirebaseFirestore newInstance( FirebaseFirestore firestore = new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app); app.addLifecycleEventListener( - /* onDeleted */ (firebaseAppName, options) -> { - firestore.shutdown(/* fromAppDeletion */ true); + (firebaseAppName, options) -> { + firestore.shutdown(/* fromAppDeletion= */ true); }); return firestore; } @@ -346,30 +346,29 @@ Task shutdown(boolean fromAppDeletion) { } /** - * Shuts down this Firestore SDK instance. + * Shuts down this FirebaseFirestore instance. * - *

    IMPORTANT: If your application is designed to run with Firestore the entire life cycle, it - * is not recommended to call `shutdown` explicitly, the shutting down happens naturally when the - * application itself is shut down. + *

    After shutdown only the {@link #clearPersistence()} method may be used. Any other method + * will throw an {@link IllegalStateException}. * - *

    For applications designed to run without Firestore from some point on (typically to save - * resources as much as possible), you may call this method. + *

    To restart after shutdown, simply create a new instance of FirebaseFirestore with {@link + * #getInstance()} or {@link #getInstance(FirebaseApp)}. * - *

    Once `shutdown` is called, trying to issue new reads/writes through this instance will - * result in {@link IllegalStateException}. Already requested reads/writes might stop resolving. - * Be sure to handle this well in your application if proceeding with Firestore shutting down is - * desired in your case. + *

    Shutdown does not cancel any pending writes and any tasks that are awaiting a response from + * the server will not be resolved. The next time you start this instance, it will resume + * attempting to send these writes to the server. * - *

    Note that {@link #clearPersistence()} is an exception, it is designed to run when the - * Firestore SDK instance is shutdown. + *

    Note: Under normal circumstances, calling shutdown() is not required. This + * method is useful only when you want to force this instance to release all of its resources or + * in combination with {@link #clearPersistence} to ensure that all local state is destroyed + * between test runs. * - *

    To re-start a Firestore SDK instance, simply create a new instance via {@link - * #getInstance()} or {@link #getInstance(FirebaseApp)}. + * @return A Task that is resolved when the instance has been successfully shut down. */ @VisibleForTesting - // TODO: Make this public and remove @VisibleForTesting + // TODO(b/135755126): Make this public and remove @VisibleForTesting Task shutdown() { - return shutdown(/* fromAppDeletion */ false); + return shutdown(/* fromAppDeletion= */ false); } @VisibleForTesting diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java index 9dece161ca5..0a45788cc57 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java @@ -43,7 +43,7 @@ class FirestoreMultiDbComponent { this.authProvider = authProvider; } - /** Provides instances of Firestore for given database names. */ + /** Provides instances of Firestore for given database IDs. */ @NonNull synchronized FirebaseFirestore get(@NonNull String databaseId) { FirebaseFirestore firestore = instances.get(databaseId); @@ -55,7 +55,7 @@ synchronized FirebaseFirestore get(@NonNull String databaseId) { } /** - * Remove the instance of a given database name from this component, such that if {@link + * Remove the instance of a given database ID from this component, such that if {@link * FirestoreMultiDbComponent#get(String)} is called again with the same name, a new instance of * {@link FirebaseFirestore} is created. * diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index 250cb24c774..fc85ac5cb21 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -147,7 +147,7 @@ public Task shutdown() { }); } - /** Has this client been shutdown. */ + /** Returns true if this client has been shutdown. */ public boolean isShutdown() { // Technically, the asyncQueue is still running, but only accepting tasks related to shutdown // or supposed to be run after shutdown. It is effectively shut down to the eyes of users. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java index 8c29f1df292..7ced19c8177 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java @@ -178,14 +178,14 @@ public static Task callTask(Executor executor, Callable + *

      *
    1. 1. Synchronized task scheduling. This is different from function 3, which is about task * execution in a single thread. *
    2. 2. Ability to do soft-shutdown: only critical tasks related to shutting Firestore SDK * down can be executed once the shutdown process initiated. *
    3. 3. Single threaded execution service, no concurrent execution among the `Runnable`s * scheduled in this Executor. - * + *
    */ private class SynchronizedShutdownAwareExecutor implements Executor { /** From 317fea79e27136c15068dfd9ea5c598292653b48 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 11 Jul 2019 11:22:16 -0400 Subject: [PATCH 05/11] fixing format in tools --- .../google/firebase/errorprone/ComponentsAppGetCheck.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java index 57e849ef510..926567a14ee 100644 --- a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java +++ b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java @@ -21,7 +21,6 @@ import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.isStatic; -import static com.google.errorprone.matchers.Matchers.isVoidType; import static com.google.errorprone.matchers.Matchers.methodHasVisibility; import static com.google.errorprone.matchers.Matchers.methodInvocation; import static com.google.errorprone.matchers.Matchers.methodIsNamed; @@ -98,11 +97,8 @@ public class ComponentsAppGetCheck extends BugChecker * */ private static final Matcher WITHIN_SHUTDOWN = - enclosingMethod( - allOf( - methodIsNamed("shutdown"))); - //methodReturns(isVoidType()))); - + enclosingMethod(allOf(methodIsNamed("shutdown"))); + // methodReturns(isVoidType()))); /** * This matches methods of the forms: From 34171e88c8eb9e328f55f84eef42c809a06966f7 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Sat, 13 Jul 2019 21:00:44 -0400 Subject: [PATCH 06/11] Stop leakages --- .../firestore/remote/GrpcCallProvider.java | 92 +++++++------- .../firebase/firestore/util/AsyncQueue.java | 117 +++++++++--------- .../firestore/util/AsyncQueueTest.java | 3 +- 3 files changed, 111 insertions(+), 101 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java index 51505926a60..fe5fa632c0a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java @@ -34,6 +34,7 @@ import io.grpc.ManagedChannelBuilder; import io.grpc.MethodDescriptor; import io.grpc.android.AndroidChannelBuilder; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; /** Manages the gRPC channel and encapsulates all SSL and gRPC initialization. */ @@ -137,48 +138,53 @@ Task> createClientCall( /** Shuts down the gRPC channel and the internal worker queue. */ void shutdown() { - channelTask.addOnCompleteListener( - asyncQueue.getExecutorForShutdown(), - task -> { - ManagedChannel channel = task.getResult(); - channel.shutdown(); - try { - // TODO(rsgowman): Investigate occasional hangs in channel.shutdown(). - // - // While running the integration tests, channel.shutdown() will occasionally timeout. - // (Typically on ~4-5 different tests, differing from one run to the next.) We should - // figure this out. But in the meantime, just use an exceptionally short timeout here - // and skip straight to shutdownNow() which works every time. (We don't support shutting - // down Firestore, so this should only be triggered from the test suite.) - if (!channel.awaitTermination(1, TimeUnit.SECONDS)) { - Logger.debug( - FirestoreChannel.class.getSimpleName(), - "Unable to gracefully shutdown the gRPC ManagedChannel. Will attempt an immediate shutdown."); - channel.shutdownNow(); - - // gRPC docs claim "Although forceful, the shutdown process is still not - // instantaneous; isTerminated() will likely return false immediately after this - // method returns." Therefore, we still need to awaitTermination() again. - if (!channel.awaitTermination(60, TimeUnit.SECONDS)) { - // Something bad has happened. We could assert, but this is just resource cleanup - // for a resource that is likely only released at the end of the execution. So - // instead, we'll just log the error. - Logger.warn( - FirestoreChannel.class.getSimpleName(), - "Unable to forcefully shutdown the gRPC ManagedChannel."); - } - } - } catch (InterruptedException e) { - // (Re-)Cancel if current thread also interrupted - channel.shutdownNow(); - - // Similar to above, something bad happened, but it's not worth asserting. Just log it. - Logger.warn( - FirestoreChannel.class.getSimpleName(), - "Interrupted while shutting down the gRPC Managed Channel"); - // Preserve interrupt status - Thread.currentThread().interrupt(); - } - }); + ManagedChannel channel = null; + try { + channel = Tasks.await(channelTask); + } catch (ExecutionException | InterruptedException e) { + Logger.warn( + FirestoreChannel.class.getSimpleName(), + "Channel is not initialized, shutdown will just do nothing."); + return; + } + + channel.shutdown(); + try { + // TODO(rsgowman): Investigate occasional hangs in channel.shutdown(). + // + // While running the integration tests, channel.shutdown() will occasionally timeout. + // (Typically on ~4-5 different tests, differing from one run to the next.) We should + // figure this out. But in the meantime, just use an exceptionally short timeout here + // and skip straight to shutdownNow() which works every time. (We don't support shutting + // down Firestore, so this should only be triggered from the test suite.) + if (!channel.awaitTermination(1, TimeUnit.SECONDS)) { + Logger.debug( + FirestoreChannel.class.getSimpleName(), + "Unable to gracefully shutdown the gRPC ManagedChannel. Will attempt an immediate shutdown."); + channel.shutdownNow(); + + // gRPC docs claim "Although forceful, the shutdown process is still not + // instantaneous; isTerminated() will likely return false immediately after this + // method returns." Therefore, we still need to awaitTermination() again. + if (!channel.awaitTermination(60, TimeUnit.SECONDS)) { + // Something bad has happened. We could assert, but this is just resource cleanup + // for a resource that is likely only released at the end of the execution. So + // instead, we'll just log the error. + Logger.warn( + FirestoreChannel.class.getSimpleName(), + "Unable to forcefully shutdown the gRPC ManagedChannel."); + } + } + } catch (InterruptedException e) { + // (Re-)Cancel if current thread also interrupted + channel.shutdownNow(); + + // Similar to above, something bad happened, but it's not worth asserting. Just log it. + Logger.warn( + FirestoreChannel.class.getSimpleName(), + "Interrupted while shutting down the gRPC Managed Channel"); + // Preserve interrupt status + Thread.currentThread().interrupt(); + } } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java index 7ced19c8177..91a89abcabb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java @@ -285,14 +285,67 @@ public synchronized void execute(Runnable command) { } } + /** Execute the command, regardless if shutdown has been initiated. */ + public void executeEvenAfterShutdown(Runnable command) { + try { + internalExecutor.execute(command); + } catch (RejectedExecutionException e) { + // The only way we can get here is if the AsyncQueue has panicked and we're now racing with + // the post to the main looper that will crash the app. + Logger.warn(AsyncQueue.class.getSimpleName(), "Refused to enqueue task after panic"); + } + } + + /** + * Run a given `Callable` on this executor, and report the result of the `Callable` in a {@link + * Task}. The `Callable` will not be run if the executor started shutting down already. + * + * @return A {@link Task} resolves when the requested `Callable` completes, or reports error + * when the `Callable` runs into exceptions. + */ + private Task executeAndReportResult(Callable task) { + final TaskCompletionSource completionSource = new TaskCompletionSource<>(); + try { + this.execute( + () -> { + try { + completionSource.setResult(task.call()); + } catch (Exception e) { + completionSource.setException(e); + throw new RuntimeException(e); + } + }); + } catch (RejectedExecutionException e) { + // The only way we can get here is if the AsyncQueue has panicked and we're now racing with + // the post to the main looper that will crash the app. + Logger.warn(AsyncQueue.class.getSimpleName(), "Refused to enqueue task after panic"); + } + return completionSource.getTask(); + } + /** * Initiate the shutdown process. Once called, the only possible way to run `Runnable`s are by * holding the `internalExecutor` reference. */ - private synchronized void initiateShutdown() { - if (!isShuttingDown) { - isShuttingDown = true; + private synchronized Task executeAndInitiateShutdown(Runnable task) { + if (isShuttingDown()) { + TaskCompletionSource source = new TaskCompletionSource<>(); + source.setResult(null); + return source.getTask(); } + + // Not shutting down yet, execute and return a Task. + Task t = + executeAndReportResult( + () -> { + task.run(); + return null; + }); + + // Mark the initiation of shut down. + isShuttingDown = true; + + return t; } /** @@ -334,14 +387,6 @@ public Executor getExecutor() { return executor; } - /** - * For tasks that need to run after shutdown has been initiated. The tasks should themselves be - * shutdown related. - */ - public Executor getExecutorForShutdown() { - return executor.internalExecutor; - } - /** Verifies that the current thread is the managed AsyncQueue thread. */ public void verifyIsCurrentThread() { Thread current = Thread.currentThread(); @@ -362,27 +407,7 @@ public void verifyIsCurrentThread() { */ @CheckReturnValue public Task enqueue(Callable task) { - return enqueueImpl(task, executor); - } - - private Task enqueueImpl(Callable task, Executor executor) { - final TaskCompletionSource completionSource = new TaskCompletionSource<>(); - try { - executor.execute( - () -> { - try { - completionSource.setResult(task.call()); - } catch (Exception e) { - completionSource.setException(e); - throw new RuntimeException(e); - } - }); - } catch (RejectedExecutionException e) { - // The only way we can get here is if the AsyncQueue has panicked and we're now racing with - // the post to the main looper that will crash the app. - Logger.warn(AsyncQueue.class.getSimpleName(), "Refused to enqueue task after panic"); - } - return completionSource.getTask(); + return executor.executeAndReportResult(task); } /** @@ -402,26 +427,11 @@ public Task enqueue(Runnable task) { /** * Queue a Runnable and immediately mark the initiation of shutdown process. Tasks queued after - * this method is called are not run unless they explicitly request via {@link - * AsyncQueue#enqueueAndForgetEvenAfterShutdown(Runnable)} or {@link - * AsyncQueue#getExecutorForShutdown()}. + * this method is called are not run unless they explicitly are requested via {@link + * AsyncQueue#enqueueAndForgetEvenAfterShutdown(Runnable)}. */ public Task enqueueAndInitiateShutdown(Runnable task) { - synchronized (executor) { - if (executor.isShuttingDown()) { - TaskCompletionSource source = new TaskCompletionSource<>(); - source.setResult(null); - return source.getTask(); - } - Task t = - enqueue( - () -> { - task.run(); - return null; - }); - executor.initiateShutdown(); - return t; - } + return executor.executeAndInitiateShutdown(task); } /** @@ -429,12 +439,7 @@ public Task enqueueAndInitiateShutdown(Runnable task) { * if shutdown has been initiated. */ public void enqueueAndForgetEvenAfterShutdown(Runnable task) { - enqueueImpl( - () -> { - task.run(); - return null; - }, - executor.internalExecutor); + executor.executeEvenAfterShutdown(task); } /** Has the shutdown process been initiated. */ diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java index 396445b2910..7b83077cc6e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java @@ -126,7 +126,7 @@ public void canManuallyDrainSpecificDelayedTasksForTesting() throws Exception { @Test public void tasksAreScheduledWithRespectToShutdown() { - expectedSteps = Arrays.asList(1, 2, 4, 6); + expectedSteps = Arrays.asList(1, 2, 4); queue.enqueueAndForget(runnableForStep(1)); // From this point on, `normal` tasks are not scheduled. Only those who explicitly request to @@ -137,6 +137,5 @@ public void tasksAreScheduledWithRespectToShutdown() { queue.enqueueAndForgetEvenAfterShutdown(runnableForStep(4)); queue.getExecutor().execute(runnableForStep(5)); - queue.getExecutorForShutdown().execute(runnableForStep(6)); } } From 74d507c227e17328167c4cbc718f12e0fc5d4541 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Sat, 13 Jul 2019 21:05:17 -0400 Subject: [PATCH 07/11] better comments --- .../com/google/firebase/firestore/util/AsyncQueue.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java index 91a89abcabb..b61ccf80145 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java @@ -179,11 +179,11 @@ public static Task callTask(Executor executor, Callable - *
  • 1. Synchronized task scheduling. This is different from function 3, which is about task + *
  • Synchronized task scheduling. This is different from function 3, which is about task * execution in a single thread. - *
  • 2. Ability to do soft-shutdown: only critical tasks related to shutting Firestore SDK - * down can be executed once the shutdown process initiated. - *
  • 3. Single threaded execution service, no concurrent execution among the `Runnable`s + *
  • Ability to do soft-shutdown: only critical tasks related to shutting Firestore SDK down + * can be executed once the shutdown process initiated. + *
  • Single threaded execution service, no concurrent execution among the `Runnable`s * scheduled in this Executor. * */ From fe02b17ad93c2e5e8766e27de8fd4d120a7e854e Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 17 Jul 2019 09:57:12 -0400 Subject: [PATCH 08/11] Handle app delete shutdown differently --- .../firebase/firestore/FirebaseFirestore.java | 18 ++++++++---------- .../firestore/FirestoreMultiDbComponent.java | 14 +++++++++++++- .../firestore/remote/GrpcCallProvider.java | 14 ++++++++++++-- .../firestore/util/AsyncQueueTest.java | 1 + .../errorprone/ComponentsAppGetCheck.java | 1 - 5 files changed, 34 insertions(+), 14 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index a075c2ea966..f28e1db1891 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -119,10 +119,6 @@ static FirebaseFirestore newInstance( FirebaseFirestore firestore = new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app); - app.addLifecycleEventListener( - (firebaseAppName, options) -> { - firestore.shutdown(/* fromAppDeletion= */ true); - }); return firestore; } @@ -335,11 +331,7 @@ public Task runBatch(@NonNull WriteBatch.Function batchFunction) { return batch.commit(); } - Task shutdown(boolean fromAppDeletion) { - if (!fromAppDeletion && this.getApp() != null) { - FirestoreMultiDbComponent component = this.getApp().get(FirestoreMultiDbComponent.class); - component.remove(this.databaseId.getDatabaseId()); - } + Task shutdownInternal() { // The client must be initialized to ensure that all subsequent API usage throws an exception. this.ensureClientConfigured(); return client.shutdown(); @@ -368,7 +360,13 @@ Task shutdown(boolean fromAppDeletion) { @VisibleForTesting // TODO(b/135755126): Make this public and remove @VisibleForTesting Task shutdown() { - return shutdown(/* fromAppDeletion= */ false); + // TODO(wuandy): This is required because test code setup Firestore without an App. We should + // eliminate that. + if (this.getApp() != null) { + FirestoreMultiDbComponent component = this.getApp().get(FirestoreMultiDbComponent.class); + component.remove(this.databaseId.getDatabaseId()); + } + return shutdownInternal(); } @VisibleForTesting diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java index 0a45788cc57..3939fd5dd42 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java @@ -18,12 +18,14 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.google.firebase.FirebaseApp; +import com.google.firebase.FirebaseAppLifecycleListener; +import com.google.firebase.FirebaseOptions; import com.google.firebase.auth.internal.InternalAuthProvider; import java.util.HashMap; import java.util.Map; /** Multi-resource container for Firestore. */ -class FirestoreMultiDbComponent { +class FirestoreMultiDbComponent implements FirebaseAppLifecycleListener { /** * A static map from instance key to FirebaseFirestore instances. Instance keys are database * names. @@ -41,6 +43,7 @@ class FirestoreMultiDbComponent { this.context = context; this.app = app; this.authProvider = authProvider; + this.app.addLifecycleEventListener(this); } /** Provides instances of Firestore for given database IDs. */ @@ -64,4 +67,13 @@ synchronized FirebaseFirestore get(@NonNull String databaseId) { synchronized void remove(@NonNull String databaseId) { instances.remove(databaseId); } + + @Override + public void onDeleted(String firebaseAppName, FirebaseOptions options) { + // Shuts down all database instances and remove them from registry map when App is deleted. + for (Map.Entry entry : instances.entrySet()) { + entry.getValue().shutdownInternal(); + instances.remove(entry.getKey()); + } + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java index fe5fa632c0a..b9e23617e82 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java @@ -138,13 +138,23 @@ Task> createClientCall( /** Shuts down the gRPC channel and the internal worker queue. */ void shutdown() { + // Handling shutdown synchronously to avoid re-enqueuing on the AsyncQueue after shutdown has + // started. ManagedChannel channel = null; try { channel = Tasks.await(channelTask); - } catch (ExecutionException | InterruptedException e) { + } catch (ExecutionException e) { Logger.warn( FirestoreChannel.class.getSimpleName(), - "Channel is not initialized, shutdown will just do nothing."); + "Channel is not initialized, shutdown will just do nothing. Channel initializing run into exception: %s", + e); + return; + } catch (InterruptedException e) { + Logger.warn( + FirestoreChannel.class.getSimpleName(), + "Interrupted while retrieving the gRPC Managed Channel"); + // Preserve interrupt status + Thread.currentThread().interrupt(); return; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java index 7b83077cc6e..b6f817ef4d8 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java @@ -137,5 +137,6 @@ public void tasksAreScheduledWithRespectToShutdown() { queue.enqueueAndForgetEvenAfterShutdown(runnableForStep(4)); queue.getExecutor().execute(runnableForStep(5)); + waitForExpectedSteps(); } } diff --git a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java index 926567a14ee..5c8417cf496 100644 --- a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java +++ b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java @@ -98,7 +98,6 @@ public class ComponentsAppGetCheck extends BugChecker */ private static final Matcher WITHIN_SHUTDOWN = enclosingMethod(allOf(methodIsNamed("shutdown"))); - // methodReturns(isVoidType()))); /** * This matches methods of the forms: From f8b33ed8b24b04c636d827acc7927197b990cff1 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 17 Jul 2019 10:57:14 -0400 Subject: [PATCH 09/11] No more error-prone hack --- .../firebase/firestore/AccessHelper.java | 11 ++++++++-- .../testutil/IntegrationTestUtil.java | 3 ++- .../firebase/firestore/FirebaseFirestore.java | 20 ++++++++++--------- .../firestore/FirestoreMultiDbComponent.java | 9 ++++++++- .../errorprone/ComponentsAppGetCheck.java | 14 +------------ 5 files changed, 31 insertions(+), 26 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java index 557c2728e2d..759e0a25301 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java @@ -31,9 +31,16 @@ public static FirebaseFirestore newFirebaseFirestore( String persistenceKey, CredentialsProvider credentialsProvider, AsyncQueue asyncQueue, - FirebaseApp firebaseApp) { + FirebaseApp firebaseApp, + FirestoreMultiDbComponent.DeregisterCommand deregisterCommand) { return new FirebaseFirestore( - context, databaseId, persistenceKey, credentialsProvider, asyncQueue, firebaseApp); + context, + databaseId, + persistenceKey, + credentialsProvider, + asyncQueue, + firebaseApp, + deregisterCommand); } /** Makes the shutdown method accessible. */ diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 3f476ba07fb..a27b3663c6e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -264,7 +264,8 @@ public static FirebaseFirestore testFirestore( persistenceKey, new EmptyCredentialsProvider(), asyncQueue, - /*firebaseApp=*/ null); + /*firebaseApp=*/ null, + /*deregisterCommand=*/ () -> {}); waitFor(AccessHelper.clearPersistence(firestore)); firestore.setFirestoreSettings(settings); firestoreStatus.put(firestore, true); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index f28e1db1891..09b1866ae10 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -61,6 +61,9 @@ public class FirebaseFirestore { private final AsyncQueue asyncQueue; private final FirebaseApp firebaseApp; private final UserDataConverter dataConverter; + // When user requests to shutdown, use this to notify `FirestoreMultiDbComponent` to deregister + // this instance. + private final FirestoreMultiDbComponent.DeregisterCommand onDeregister; private FirebaseFirestoreSettings settings; private volatile FirestoreClient client; @@ -94,7 +97,8 @@ static FirebaseFirestore newInstance( @NonNull Context context, @NonNull FirebaseApp app, @Nullable InternalAuthProvider authProvider, - @NonNull String database) { + @NonNull String database, + @NonNull FirestoreMultiDbComponent.DeregisterCommand onDeregister) { String projectId = app.getOptions().getProjectId(); if (projectId == null) { throw new IllegalArgumentException("FirebaseOptions.getProjectId() cannot be null"); @@ -118,7 +122,8 @@ static FirebaseFirestore newInstance( String persistenceKey = app.getName(); FirebaseFirestore firestore = - new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app); + new FirebaseFirestore( + context, databaseId, persistenceKey, provider, queue, app, onDeregister); return firestore; } @@ -129,7 +134,8 @@ static FirebaseFirestore newInstance( String persistenceKey, CredentialsProvider credentialsProvider, AsyncQueue asyncQueue, - @Nullable FirebaseApp firebaseApp) { + @Nullable FirebaseApp firebaseApp, + FirestoreMultiDbComponent.DeregisterCommand onDeregister) { this.context = checkNotNull(context); this.databaseId = checkNotNull(checkNotNull(databaseId)); this.dataConverter = new UserDataConverter(databaseId); @@ -138,6 +144,7 @@ static FirebaseFirestore newInstance( this.asyncQueue = checkNotNull(asyncQueue); // NOTE: We allow firebaseApp to be null in tests only. this.firebaseApp = firebaseApp; + this.onDeregister = onDeregister; settings = new FirebaseFirestoreSettings.Builder().build(); } @@ -360,12 +367,7 @@ Task shutdownInternal() { @VisibleForTesting // TODO(b/135755126): Make this public and remove @VisibleForTesting Task shutdown() { - // TODO(wuandy): This is required because test code setup Firestore without an App. We should - // eliminate that. - if (this.getApp() != null) { - FirestoreMultiDbComponent component = this.getApp().get(FirestoreMultiDbComponent.class); - component.remove(this.databaseId.getDatabaseId()); - } + onDeregister.execute(); return shutdownInternal(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java index 3939fd5dd42..bcba1112aac 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java @@ -26,6 +26,11 @@ /** Multi-resource container for Firestore. */ class FirestoreMultiDbComponent implements FirebaseAppLifecycleListener { + + public interface DeregisterCommand { + void execute(); + } + /** * A static map from instance key to FirebaseFirestore instances. Instance keys are database * names. @@ -51,7 +56,9 @@ class FirestoreMultiDbComponent implements FirebaseAppLifecycleListener { synchronized FirebaseFirestore get(@NonNull String databaseId) { FirebaseFirestore firestore = instances.get(databaseId); if (firestore == null) { - firestore = FirebaseFirestore.newInstance(context, app, authProvider, databaseId); + firestore = + FirebaseFirestore.newInstance( + context, app, authProvider, databaseId, () -> this.remove(databaseId)); instances.put(databaseId, firestore); } return firestore; diff --git a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java index 5c8417cf496..b42c5dbfa48 100644 --- a/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java +++ b/tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java @@ -87,18 +87,6 @@ public class ComponentsAppGetCheck extends BugChecker isSupertypeOf( state -> ASTHelpers.getType(state.findEnclosing(ClassTree.class)))))); - /** - * This matches methods of the forms: - * - * 
    {@code
-   * class Foo {
-   *     void shutdown();
-   * }
-   *
    - */ - private static final Matcher WITHIN_SHUTDOWN = - enclosingMethod(allOf(methodIsNamed("shutdown"))); - /** * This matches methods of the forms: * @@ -126,7 +114,7 @@ public class ComponentsAppGetCheck extends BugChecker enclosingClass(hasAnnotation("org.junit.runner.RunWith")); private static final Matcher ALLOWED_USAGES = - anyOf(WITHIN_GET_INSTANCE, WITHIN_GET_INSTANCE_IMPL, WITHIN_SHUTDOWN, WITHIN_JUNIT_TEST); + anyOf(WITHIN_GET_INSTANCE, WITHIN_GET_INSTANCE_IMPL, WITHIN_JUNIT_TEST); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { From d3141b2f4abadf5a132c031b84462573f6524a8d Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 17 Jul 2019 14:56:04 -0400 Subject: [PATCH 10/11] Change how to interact with MultiDbComponent. --- .../firebase/firestore/AccessHelper.java | 4 ++-- .../testutil/IntegrationTestUtil.java | 2 +- .../firebase/firestore/FirebaseFirestore.java | 18 ++++++++++++------ .../firestore/FirestoreMultiDbComponent.java | 14 +++++--------- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java index 759e0a25301..165f8085aed 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java @@ -32,7 +32,7 @@ public static FirebaseFirestore newFirebaseFirestore( CredentialsProvider credentialsProvider, AsyncQueue asyncQueue, FirebaseApp firebaseApp, - FirestoreMultiDbComponent.DeregisterCommand deregisterCommand) { + FirebaseFirestore.InstanceRegistry instanceRegistry) { return new FirebaseFirestore( context, databaseId, @@ -40,7 +40,7 @@ public static FirebaseFirestore newFirebaseFirestore( credentialsProvider, asyncQueue, firebaseApp, - deregisterCommand); + instanceRegistry); } /** Makes the shutdown method accessible. */ diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index a27b3663c6e..8afb61adf1f 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -265,7 +265,7 @@ public static FirebaseFirestore testFirestore( new EmptyCredentialsProvider(), asyncQueue, /*firebaseApp=*/ null, - /*deregisterCommand=*/ () -> {}); + /*instanceRegistry=*/ (dbId) -> {}); waitFor(AccessHelper.clearPersistence(firestore)); firestore.setFirestoreSettings(settings); firestoreStatus.put(firestore, true); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 09b1866ae10..a13a9724164 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -51,6 +51,12 @@ @PublicApi public class FirebaseFirestore { + /** Provides a registry management interface for {@code FirebaseFirestore} instances. */ + public interface InstanceRegistry { + /** Removes the Firestore instance with given name from registry. */ + void remove(@NonNull String databaseId); + } + private static final String TAG = "FirebaseFirestore"; private final Context context; // This is also used as private lock object for this instance. There is nothing inherent about @@ -63,7 +69,7 @@ public class FirebaseFirestore { private final UserDataConverter dataConverter; // When user requests to shutdown, use this to notify `FirestoreMultiDbComponent` to deregister // this instance. - private final FirestoreMultiDbComponent.DeregisterCommand onDeregister; + private final InstanceRegistry instanceRegistry; private FirebaseFirestoreSettings settings; private volatile FirestoreClient client; @@ -98,7 +104,7 @@ static FirebaseFirestore newInstance( @NonNull FirebaseApp app, @Nullable InternalAuthProvider authProvider, @NonNull String database, - @NonNull FirestoreMultiDbComponent.DeregisterCommand onDeregister) { + @NonNull InstanceRegistry instanceRegistry) { String projectId = app.getOptions().getProjectId(); if (projectId == null) { throw new IllegalArgumentException("FirebaseOptions.getProjectId() cannot be null"); @@ -123,7 +129,7 @@ static FirebaseFirestore newInstance( FirebaseFirestore firestore = new FirebaseFirestore( - context, databaseId, persistenceKey, provider, queue, app, onDeregister); + context, databaseId, persistenceKey, provider, queue, app, instanceRegistry); return firestore; } @@ -135,7 +141,7 @@ static FirebaseFirestore newInstance( CredentialsProvider credentialsProvider, AsyncQueue asyncQueue, @Nullable FirebaseApp firebaseApp, - FirestoreMultiDbComponent.DeregisterCommand onDeregister) { + InstanceRegistry instanceRegistry) { this.context = checkNotNull(context); this.databaseId = checkNotNull(checkNotNull(databaseId)); this.dataConverter = new UserDataConverter(databaseId); @@ -144,7 +150,7 @@ static FirebaseFirestore newInstance( this.asyncQueue = checkNotNull(asyncQueue); // NOTE: We allow firebaseApp to be null in tests only. this.firebaseApp = firebaseApp; - this.onDeregister = onDeregister; + this.instanceRegistry = instanceRegistry; settings = new FirebaseFirestoreSettings.Builder().build(); } @@ -367,7 +373,7 @@ Task shutdownInternal() { @VisibleForTesting // TODO(b/135755126): Make this public and remove @VisibleForTesting Task shutdown() { - onDeregister.execute(); + instanceRegistry.remove(this.getDatabaseId().getDatabaseId()); return shutdownInternal(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java index bcba1112aac..d52134e7551 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java @@ -25,11 +25,8 @@ import java.util.Map; /** Multi-resource container for Firestore. */ -class FirestoreMultiDbComponent implements FirebaseAppLifecycleListener { - - public interface DeregisterCommand { - void execute(); - } +class FirestoreMultiDbComponent + implements FirebaseAppLifecycleListener, FirebaseFirestore.InstanceRegistry { /** * A static map from instance key to FirebaseFirestore instances. Instance keys are database @@ -56,9 +53,7 @@ public interface DeregisterCommand { synchronized FirebaseFirestore get(@NonNull String databaseId) { FirebaseFirestore firestore = instances.get(databaseId); if (firestore == null) { - firestore = - FirebaseFirestore.newInstance( - context, app, authProvider, databaseId, () -> this.remove(databaseId)); + firestore = FirebaseFirestore.newInstance(context, app, authProvider, databaseId, this); instances.put(databaseId, firestore); } return firestore; @@ -71,7 +66,8 @@ synchronized FirebaseFirestore get(@NonNull String databaseId) { * *

    It is a no-op if there is no instance associated with the given database name. */ - synchronized void remove(@NonNull String databaseId) { + @Override + public synchronized void remove(@NonNull String databaseId) { instances.remove(databaseId); } From 701fcef2de51f3c117a7168ac1d7cbf3d1757d2c Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 17 Jul 2019 15:19:01 -0400 Subject: [PATCH 11/11] add missing sync keyword --- .../google/firebase/firestore/FirestoreMultiDbComponent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java index d52134e7551..498a9da9be8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java @@ -72,7 +72,7 @@ public synchronized void remove(@NonNull String databaseId) { } @Override - public void onDeleted(String firebaseAppName, FirebaseOptions options) { + public synchronized void onDeleted(String firebaseAppName, FirebaseOptions options) { // Shuts down all database instances and remove them from registry map when App is deleted. for (Map.Entry entry : instances.entrySet()) { entry.getValue().shutdownInternal();