Prepare `shutdown` to be a public api. #589

ghost · 2019-07-08T20:01:51Z

FirebaseApp delete calls firestore shutdown.
Deregister firestore when shutdown is called, new instance can then be registered.
Synchronize AsyncQueue.Executor scheduling. Also make it stop handling most of schedule requests once it is shutdown, only tasks originated from shutdown itself and clearPersistence can proceed.

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java

wilhuff · 2019-07-10T18:14:44Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

          }
        });
  }

+  /** Has this client been shutdown. */


Use a verb phrase that describes what the method does as if the sentence starts with "This method" (see http://go/java-practices/javadoc#summary_methods).

e.g.

/** Returns true if this client has been shut down. */

wilhuff · 2019-07-10T18:16:37Z

firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java

-   * so that it's possible later to make assertions about executing on the correct thread.
+   * A wrapper around a {@link ScheduledThreadPoolExecutor} class that provides:
+   *
+   * <ul>


Instead of <ul> with manual numbers, consider just using an ordered list (<ol>).

In javadoc this will now read:

1. 1. Synchronized ... 2. 2. Ability ... 3. 3. Single ...

<ol> makes each <li> insert a number instead of a bullet.

firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java

wilhuff

This is looking a lot better.

wilhuff · 2019-07-16T19:19:55Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

-    return new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app);
+    FirebaseFirestore firestore =
+        new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app);
+    app.addLifecycleEventListener(


This is the wrong layer at which to do this. newInstance shouldn't be registering a listener for this individual instance.

Instead, have FirestoreMultiDbComponent implement FirebaseAppLifecycleListener itself and have it register once, as the final action in its constructor. In the onDeleted implementation, iterate through all instances and shut down the instances.

wilhuff · 2019-07-16T19:24:41Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

-  @VisibleForTesting
-  Task<Void> shutdown() {
+  Task<Void> shutdown(boolean fromAppDeletion) {
+    if (!fromAppDeletion && this.getApp() != null) {


It would be clearer if we split this apart:

Public shutdown could just unconditionally navigate to to the component and remove this instance.

If App is deleted, the lifecycle implementation in FirestoreMultiDbComponent will take care of dropping from that direction.

Have this method be package protected and not take a parameter. Call it something like shutdownInternal. (The precedent would be e.g. disableNetwork and disableNetworkInternal in RemoteStore.

Have this method only take the actions outside the if block.

wilhuff · 2019-07-16T20:09:56Z

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java

+    ManagedChannel channel = null;
+    try {
+      channel = Tasks.await(channelTask);
+    } catch (ExecutionException | InterruptedException e) {


If you catch InterruptedException you need to re-interrupt the current thread. That means that you can't handle these cases together.

Also, if there was an ExecutionException this may be our only opportunity to log what happened, so we should log the the exception too.

wilhuff · 2019-07-16T20:11:48Z

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java

-            Thread.currentThread().interrupt();
-          }
-        });
+    ManagedChannel channel = null;


This needs a comment about why we're handling it synchronously because this is otherwise a suspicious idea.

Something like: "Handle shutdown synchronously to avoid re-enqueuing on the AsyncQueue after shutdown has started."

wilhuff · 2019-07-16T20:20:55Z

firebase-firestore/src/test/java/com/google/firebase/firestore/util/AsyncQueueTest.java

+    queue.enqueueAndForget(runnableForStep(3));
+    queue.enqueueAndForgetEvenAfterShutdown(runnableForStep(4));
+
+    queue.getExecutor().execute(runnableForStep(5));


This test makes no assertions. It seems like this should assert that 1, 2, and 4 completed.

Added waitForExpectedSteps, this plus the assertions in runnableForStep should be sufficient now.

wilhuff · 2019-07-16T20:22:50Z

tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java

+   */
+  private static final Matcher<ExpressionTree> WITHIN_SHUTDOWN =
+      enclosingMethod(allOf(methodIsNamed("shutdown")));
+  // methodReturns(isVoidType())));


Commented out code?

wilhuff · 2019-07-16T20:24:49Z

tools/errorprone/src/main/java/com/google/firebase/errorprone/ComponentsAppGetCheck.java

@@ -87,6 +87,19 @@
                  isSupertypeOf(
                      state -> ASTHelpers.getType(state.findEnclosing(ClassTree.class))))));

+  /**


This needs review by someone on the Android Core team.

Asked Vlad to review.

vkryachko · 2019-07-17T13:45:38Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

-  Task<Void> shutdown() {
+  Task<Void> shutdown(boolean fromAppDeletion) {
+    if (!fromAppDeletion && this.getApp() != null) {
+      FirestoreMultiDbComponent component = this.getApp().get(FirestoreMultiDbComponent.class);


You should not modify the errorprone check for "convenience" as this convenience comes at a price:

You're essentially gaining access to any undeclared component dependency. This is something we(acore) want teams to do only if absolutely necessary and only as a temporary measure that they should consider tech debt.

It makes the code harder to unit test as you're no longer getting everything you need via a constructor but effectively gaining access to services through another object you have access to via a "service locator" pattern(see https://github.com/google/guice/wiki/InjectOnlyDirectDependencies)

Additionally, in this particular case, you are coupling this class to FirestoreMultiDbComponent imo unnecessarily

That said, I'd recommend something along the following lines:

add a constructor parameter to FirebaseFirestore in the form of Consumer<String> onShutdown(or equivalent as it is java8 only)

have FirestoreMultiDbComponent inject the Consumer<String> into Firestore as a simple lambda that removes the instance based on the provided String dbId

have firestore call onShutdown when appropriate.

Basically what you suggested, slight tweak.

vkryachko · 2019-07-17T15:00:20Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java

-class FirestoreMultiDbComponent {
+class FirestoreMultiDbComponent implements FirebaseAppLifecycleListener {
+
+  public interface DeregisterCommand {


java.lang.Runnable instead?

That's a good idea.

Alternatively, you could just make an interface that contains remove(String databaseId) such that FirestoreMultiDbComponent could just implement it. This removes the extra capture.

In any case, the interface describing this behavior should not be nested in FirestoreMultiDbComponent because that means FirebaseFirestore still ends up having an awareness of that type. Instead nest the interface in FirebaseFirestore.

Something like this is what I mean:

class FirebaseFiresore { // ... interface InstanceRegistry { void remove(@NonNull String databaseId); } }

FirestoreMultiDbComponent can implement this interface directly.

vkryachko · 2019-07-17T15:02:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java

@@ -41,16 +48,39 @@
    this.context = context;
    this.app = app;
    this.authProvider = authProvider;
+    this.app.addLifecycleEventListener(this);


Just a consideration: app.delete() is not a documented API and no other SDK integrates with it(hence it is effectively almost a noop), I'll defer to your judgement on whether you want to integrate with it

During the API discussion about this, the consensus emerged that shutdown must not leave a broken instance registered and that app deletion should mean shutdown of the Firestore instance.

While this hasn't been made public or documented on Android specifically, that's a localized problem. We want our ports to behave identically to the extent that we can arrange it. If this ever becomes a documented API we'll be ready. In other words: this will be the behavior on the other ports so might as well do the same here.

vkryachko

LGTM, but make sure there is no data race in onDeleted

vkryachko · 2019-07-17T19:11:15Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java

@@ -41,16 +48,39 @@
    this.context = context;
    this.app = app;
    this.authProvider = authProvider;
+    this.app.addLifecycleEventListener(this);


vkryachko · 2019-07-17T19:12:17Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java

+  }
+
+  @Override
+  public void onDeleted(String firebaseAppName, FirebaseOptions options) {


Shouldn't this method also synchronize its access to the map?

Fixed. Thanks.

wilhuff

LGTM after addressing @vkryachko's synchronization comment.

wu-hui added 3 commits July 4, 2019 11:19

Deregister firestore when shutdown + app.delete calls shutdown

fa5e624

synchronized scheduling + soft shutdown

9ba6ff3

Add comments and more tests

835ddc8

googlebot added the cla: yes Override cla label Jul 8, 2019

google-oss-bot added the size/L label Jul 8, 2019

wilhuff self-assigned this Jul 9, 2019

wilhuff suggested changes Jul 10, 2019

View reviewed changes

wilhuff assigned ghost and unassigned wilhuff Jul 10, 2019

wilhuff added the api: firestore label Jul 10, 2019

wu-hui added 2 commits July 11, 2019 10:50

addressing comments #1

e7e3ea1

fixing format in tools

317fea7

ghost assigned wilhuff and unassigned ghost Jul 11, 2019

wilhuff assigned ghost and unassigned wilhuff Jul 12, 2019

Stop leakages

34171e8

google-oss-bot added size/XL and removed size/L labels Jul 14, 2019

better comments

74d507c

ghost assigned wilhuff and unassigned ghost Jul 16, 2019

wilhuff reviewed Jul 16, 2019

View reviewed changes

wilhuff assigned ghost and unassigned wilhuff Jul 16, 2019

ghost requested a review from vkryachko July 17, 2019 13:20

vkryachko requested changes Jul 17, 2019

View reviewed changes

wu-hui added 2 commits July 17, 2019 09:57

Handle app delete shutdown differently

fe02b17

No more error-prone hack

f8b33ed

vkryachko reviewed Jul 17, 2019

View reviewed changes

ghost assigned wilhuff and unassigned ghost Jul 17, 2019

wilhuff assigned ghost and unassigned wilhuff Jul 17, 2019

Change how to interact with MultiDbComponent.

d3141b2

ghost assigned vkryachko and wilhuff and unassigned ghost Jul 17, 2019

vkryachko approved these changes Jul 17, 2019

View reviewed changes

wilhuff approved these changes Jul 17, 2019

View reviewed changes

wilhuff assigned ghost and unassigned vkryachko and wilhuff Jul 17, 2019

add missing sync keyword

701fcef

ghost assigned vkryachko and unassigned ghost Jul 17, 2019

ghost merged commit 8152ab8 into master Jul 17, 2019

rlazo deleted the MakeShutdownPublic branch September 27, 2019 14:55

firebase locked and limited conversation to collaborators Oct 9, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare `shutdown` to be a public api. #589

Prepare `shutdown` to be a public api. #589

ghost commented Jul 8, 2019

wilhuff Jul 10, 2019

ghost Jul 11, 2019

wilhuff Jul 10, 2019

ghost Jul 11, 2019

wilhuff Jul 12, 2019

wilhuff left a comment

wilhuff Jul 16, 2019

ghost Jul 17, 2019

wilhuff Jul 16, 2019

ghost Jul 17, 2019

wilhuff Jul 16, 2019

ghost Jul 17, 2019

wilhuff Jul 16, 2019

ghost Jul 17, 2019

wilhuff Jul 16, 2019

ghost Jul 17, 2019

wilhuff Jul 16, 2019

ghost Jul 17, 2019

wilhuff Jul 16, 2019

ghost Jul 17, 2019

vkryachko Jul 17, 2019

ghost Jul 17, 2019

vkryachko Jul 17, 2019

wilhuff Jul 17, 2019

ghost Jul 17, 2019

vkryachko Jul 17, 2019

wilhuff Jul 17, 2019

vkryachko Jul 17, 2019

vkryachko left a comment

vkryachko Jul 17, 2019

vkryachko Jul 17, 2019

ghost Jul 17, 2019

wilhuff left a comment •

edited

Loading

Prepare shutdown to be a public api. #589

Prepare shutdown to be a public api. #589

Conversation

ghost commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkryachko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment • edited Loading

Choose a reason for hiding this comment

Prepare `shutdown` to be a public api. #589

Prepare `shutdown` to be a public api. #589

wilhuff left a comment •

edited

Loading