port public shutdown to web sdk. #2045

ghost · 2019-08-01T23:51:27Z

No description provided.

…se/firebase-js-sdk into wuandy/PortPublicShutdown

mikelehen

Mostly looks good from a porting perspective. I flagged a few nits.

That said, (and I'm sorry for providing this feedback now when it's pretty late) I'm not super psyched about how much this client shutdown feature is bleeding into the AsyncQueue. We now have the following on the AsyncQueue:

isShuttingDown which indicates the client is shutdown / shutting down (but the AsyncQueue is still functional as long as you call the right version of its methods)`
enqueueEvenAfterShutdown / enqueueAndForgetEvenAfterShutdown - Same as the normal enqueue functions except they don't no-op once the queue is shutdown.
enqueueAndInitializeShutdown() same as the normal enqueue functions except it also sets the shutdown flag.
enqueue() goes into a special no-op mode (returns Promises that don't resolve) once the queue is shutting down.

This is more churn on AsyncQueue than I would have expected and it feels like AsyncQueue is being contorted to meet the client's shutdown needs in a way that doesn't feel 100% natural to me.

I find myself wondering why the AsyncQueue needs to be intimately aware of client shutdown instead of the client just shutting down its components and entering a mode where it no longer queues new work on the queue? Did we deem this harder to implement? Less safe? Some combination? It seems like it would be cleaner to me, but if you / @wilhuff / @schmidt-sebastian have explored this sufficiently already, I'm okay just moving forward rather than revisiting.

mikelehen · 2019-08-02T23:09:34Z

packages/firebase/index.d.ts

-    INTERNAL: { delete: () => Promise<void> };
+    INTERNAL: {
+      delete: () => Promise<void>;
+      shutdown: () => Promise<void>;


I guess you are putting these here so as not to publicly expose them? That isn't completely crazy, but in the past what we've done is:

Name the method with a leading underscore to indicate it's "private", e.g. _shutdown()

not added it to our .d.ts files so that folks using TypeScript won't be able to use it (unless they cast to any or something).

E.g. that's what Brian did for clearPersistence(). here's the change where we finally made it public for real: 7a15e7e

INTERNAL is currently meant for internal APIs meant for FirebaseApp to call (e.g. FirebaseApp will call our delete() method when somebody calls FirebaseApp.delete() but we don't want users to call our delete() method directly).

mikelehen · 2019-08-02T23:15:51Z

packages/firestore/src/api/database.ts

+      return this.INTERNAL.delete();
+    },
+
+    isShutdown: (): boolean => {


This should probably be a readonly property instead of a method... which may be hard/impossible to do since it's on INTERNAL which isn't a class. If you move it to Firestore (where it will eventually live), it should look something like

firebase-js-sdk/packages/firestore/src/api/database.ts

Line 529 in 3c3ddb4

get app(): FirebaseApp {

mikelehen · 2019-08-02T23:18:12Z

packages/firestore/src/api/database.ts

+    },
+
+    isShutdown: (): boolean => {
+      return this._firestoreClient!.clientShutdown;


This will probably fail if you do firebase.firestore().INTERNAL.isShutdown(). since _firestoreClient doesn't get initialized until you call a method on the Firestore instance. So this method probably needs to either call this.ensureClientConfigured(); to initialize _firestoreClient or else tolerate an uninitialized _firestoreClient

mikelehen · 2019-08-02T23:19:00Z

packages/firestore/src/api/database.ts

+     */
+    // TODO(b/135755126): make this public.
+    shutdown: (): Promise<void> => {
+      (this._config.firebaseApp as _FirebaseApp)._removeServiceInstance(


this.app ?

mikelehen · 2019-08-02T23:51:18Z

packages/firestore/src/util/async_queue.ts

@@ -199,6 +203,12 @@ export class AsyncQueue {
  // assertion sanity-checks.
  private operationInProgress = false;

+  // Is this AsyncQueue being shut down? If true, this instance will not enqueue
+  // any new operations, Promises from enqueue requests will not resolve.
+  get isShuttingDown(): boolean {


I assume this name was decided in previous ports, in which case, feel free to just leave it. But I would find "isShutdown" clearer than "isShuttingDown" since the latter sounds like a very transitive state during the actual shutdown process itself, rather than a permanent state.

Yeah, this is from other ports. The reason is async queue is not actually shutdown..so i picked this name originally, it is leaking the details a bit. But async queue is an internal class used frequently, maybe it's not too bad to indicate how it is implemented on the name.

FirestoreClient has clientShutdown instead, because that is one abstraction up.

mikelehen · 2019-08-05T14:47:38Z

packages/firestore/src/api/database.ts

+     *
+     * Shutdown does not cancel any pending writes and any promises 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.


This should probably be "If you have persistence enabled, the next time you start ..."

Done. Although i might need to back port this.

mikelehen · 2019-08-05T15:06:40Z

packages/firestore/src/util/async_queue.ts

+    op: () => Promise<T>
+  ): Promise<T> {
+    this.verifyNotFailed();
+    // tslint:disable-next-line:no-floating-promises


this shouldn't be necessary.

Actually yarn lint complains about this.

mikelehen · 2019-08-05T15:18:35Z

packages/firestore/src/util/async_queue.ts

+    this.verifyNotFailed();
+    if (this._isShuttingDown) {
+      // Return a Promise resolves right away if it is already shutdown.
+      return new Promise<T>(resolve => resolve(undefined));


You would normally use Promise.resolve() for this... but I see that gives a type error because undefined isn't convertible to T, which is a valid complaint. So I think if this is the behavior we want, the right thing to do here is return Promise.resolve(undefined as T) but we are subverting the type system (T likely isn't supposed to include undefined).

But as it happens, the only code we pass to enqueueAndInitilizeShutdown() returns Promise<void> so we should probably just make this method non-generic, which solves our problems. Finally, you could:

Make this method async so you can use await and the compiler will automatically make all code paths return Promise (i.e. it'll generate a Promise.resolve(undefined) for the no-op case.

Use your enqueueEvenAfterShutdown() so you don't need to do the enqueueInternal() / isShuttingDown=true reverse ordering trick.

So you end up with:

async enqueueAndInitilizeShutdown(op: () => Promise<void>): Promise<void> { this.verifyNotFailed(); if (!this._isShuttingDown) { this._isShuttingDown = true; await this.enqueueEvenAfterShutdown(op); } }

This will cause an error in your async_queue.test.ts test which you can fix by changing:

queue.enqueueAndInitilizeShutdown(() => doStep(2));

to:

queue.enqueueAndInitilizeShutdown(async () => { doStep(2); });

or you could just change doStop() to return Promise<void> (as I mention again below).

Thanks..much better!

mikelehen · 2019-08-05T15:44:17Z

packages/firestore/test/unit/util/async_queue.test.ts

+    const doStep = (n: number): Promise<number> =>
+      defer(() => {
+        const result = completedSteps.push(n);
+        return result;


Since you're not using the result, you could drop this return and change this to return Promise<void>.

mikelehen · 2019-08-05T15:51:10Z

packages/firestore/src/core/firestore_client.ts

-        // auth tokens.
-        this.credentials.removeChangeListener();
-        this._clientShutdown = true;
+    return this.asyncQueue.enqueueAndInitilizeShutdown(async () => {


Initilize => Initialize

FWIW, If you are using VS Code (if not, disregard), there's a "Code Spell Checker" extension from "Street Side Software" which is handy.

Actually, Initialize => Initiate to match the other ports.

wilhuff

The high level concern you're raising here is a valid one but the contortions come from the fact that we want clearPersistence to be able to run after shutdown. You're welcome to propose an alternative, but I think at this point you have to help port it :-).

wilhuff · 2019-08-05T16:05:12Z

packages/firestore/src/core/firestore_client.ts

-        // auth tokens.
-        this.credentials.removeChangeListener();
-        this._clientShutdown = true;
+    return this.asyncQueue.enqueueAndInitilizeShutdown(async () => {


Actually, Initialize => Initiate to match the other ports.

mikelehen · 2019-08-05T20:47:45Z

Fair enough. My naive alternative is basically "Don't shut down the queue" since it's not actually shutting down and we still actively need it (for clearPersistence())". But I don't know what breaks if you don't shut it down, and it's probably not worth re-exploring this, so I'll defer my criticism until a point in time where I feel motivated to explore it myself. :)

wilhuff · 2019-08-05T20:52:46Z

Well, the thing that we want is in a few parts:

clearPersistence should be able to run after shutdown
shutdown should be able to run after shutdown (and have no effect)
anything else should fail
callbacks submitted to other APIs should not fail.

So far we've chosen to push the is shutdown check down into the async queue, but another way to go would be to push it up into FirestoreClient.

ghost

I actually started the task by doing the check in FirestoreClient, but i could not be sure that is enough to stop all activities going on with the SDK, it might be true but my glossing over code base does not seem to support it. I believe there will be activities going on after shut down, and i am not sure if we can tolerate those or not.

I decided to go into async queue and make the change there to be certain that we can stop all activities in a more controlled manner.

ghost · 2019-08-06T01:24:29Z

packages/firebase/index.d.ts

-    INTERNAL: { delete: () => Promise<void> };
+    INTERNAL: {
+      delete: () => Promise<void>;
+      shutdown: () => Promise<void>;


ghost · 2019-08-06T01:25:08Z

packages/firestore/src/api/database.ts

+     * Shuts down this Firestore instance.
+     *
+     * After shutdown only the `clearPersistence()` method may be used. Any other method
+     * will throw an `FirestoreError`.


ghost · 2019-08-06T01:25:16Z

packages/firestore/src/api/database.ts

+     * will throw an `FirestoreError`.
+     *
+     * To restart after shutdown, simply create a new instance of FirebaseFirestore with
+     * `Firebase.firestore()`.


ghost · 2019-08-06T01:26:54Z

packages/firestore/src/api/database.ts

+     */
+    // TODO(b/135755126): make this public.
+    shutdown: (): Promise<void> => {
+      (this._config.firebaseApp as _FirebaseApp)._removeServiceInstance(


ghost · 2019-08-06T01:27:10Z

packages/firestore/src/api/database.ts

+     *
+     * Shutdown does not cancel any pending writes and any promises 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.


Done. Although i might need to back port this.

ghost · 2019-08-06T01:37:48Z

packages/firestore/src/core/firestore_client.ts

-        // auth tokens.
-        this.credentials.removeChangeListener();
-        this._clientShutdown = true;
+    return this.asyncQueue.enqueueAndInitilizeShutdown(async () => {


ghost · 2019-08-06T01:39:55Z

packages/firestore/test/unit/util/async_queue.test.ts

+    const doStep = (n: number): Promise<number> =>
+      defer(() => {
+        const result = completedSteps.push(n);
+        return result;


ghost · 2019-08-06T01:42:18Z

packages/firestore/src/util/async_queue.ts

+    op: () => Promise<T>
+  ): Promise<T> {
+    this.verifyNotFailed();
+    // tslint:disable-next-line:no-floating-promises


ghost · 2019-08-06T01:46:09Z

packages/firestore/src/util/async_queue.ts

@@ -199,6 +203,12 @@ export class AsyncQueue {
  // assertion sanity-checks.
  private operationInProgress = false;

+  // Is this AsyncQueue being shut down? If true, this instance will not enqueue
+  // any new operations, Promises from enqueue requests will not resolve.
+  get isShuttingDown(): boolean {


Yeah, this is from other ports. The reason is async queue is not actually shutdown..so i picked this name originally, it is leaking the details a bit. But async queue is an internal class used frequently, maybe it's not too bad to indicate how it is implemented on the name.

FirestoreClient has clientShutdown instead, because that is one abstraction up.

ghost · 2019-08-06T01:47:47Z

packages/firestore/src/util/async_queue.ts

+    this.verifyNotFailed();
+    if (this._isShuttingDown) {
+      // Return a Promise resolves right away if it is already shutdown.
+      return new Promise<T>(resolve => resolve(undefined));


Thanks..much better!

mikelehen

Couple tiny things left...

mikelehen · 2019-08-06T17:20:21Z

packages/firestore/src/api/database.ts

@@ -447,6 +447,39 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
    return deferred.promise;
  }

+  /**
+     * Shuts down this Firestore instance.


leading spaces seems off... If you run yarn lint from packages/firestore, it should complain about this (and it's complaining in the CI build too).

mikelehen · 2019-08-06T17:40:50Z

packages/firestore/test/integration/api/database.test.ts

@@ -1073,7 +1073,7 @@ apiDescribe('Database', (persistence: boolean) => {
  it('can start a new instance after shut down', async () => {
    return withTestDoc(persistence, async docRef => {
      const firestore = docRef.firestore;
-      await firestore.INTERNAL.shutdown();
+      await (firestore as any)._shutdown();


any is disallowed by the linter (again, running yarn lint from packages/firestore will complain). So you'll need a lint suppression. I'd recommend trying not to repeat it over and over. So you could create a helper like Brian did for clearPersistence(): 7a15e7e#diff-1422c3d56f022a40f00b40d264351ffdL109

mikelehen

LGTM

Feiyang1

LGTM for the _removeServiceInstance() piece

wu-hui added 7 commits July 30, 2019 14:02

pending firebaseApp change

8dd846d

[AUTOMATED]: Prettier Code Styling

2ab4b34

Looks to be working.

c9e1495

browser passes/node fails

efffc5e

[AUTOMATED]: Prettier Code Styling

a07588c

Merge branch 'wuandy/PortPublicShutdown' of https://github.com/fireba…

3e6bfca

…se/firebase-js-sdk into wuandy/PortPublicShutdown

add settings for new instance

692dfa3

ghost added the api: firestore label Aug 1, 2019

ghost requested a review from mikelehen August 1, 2019 23:51

ghost requested review from Feiyang1, hiranya911 and hsubox76 as code owners August 1, 2019 23:51

final self review.

1438df0

ghost assigned mikelehen and Feiyang1 Aug 2, 2019

mikelehen suggested changes Aug 5, 2019

View reviewed changes

mikelehen assigned ghost and unassigned mikelehen Aug 5, 2019

wilhuff reviewed Aug 5, 2019

View reviewed changes

wu-hui added 2 commits August 5, 2019 21:58

addressing comments #1

eeec389

[AUTOMATED]: Prettier Code Styling

45c65a1

ghost commented Aug 6, 2019

View reviewed changes

ghost assigned mikelehen and unassigned ghost Aug 6, 2019

mikelehen suggested changes Aug 6, 2019

View reviewed changes

mikelehen assigned ghost and unassigned mikelehen Aug 6, 2019

address comments #2

59ea6b4

ghost assigned mikelehen and unassigned ghost Aug 6, 2019

mikelehen approved these changes Aug 6, 2019

View reviewed changes

mikelehen assigned ghost and unassigned mikelehen Aug 6, 2019

Feiyang1 approved these changes Aug 6, 2019

View reviewed changes

Merge branch 'master' into wuandy/PortPublicShutdown

8d957d7

ghost merged commit 80d0846 into master Aug 7, 2019

firebase locked and limited conversation to collaborators Oct 8, 2019

This pull request was closed.

port public shutdown to web sdk. #2045

port public shutdown to web sdk. #2045

Conversation

ghost commented Aug 1, 2019

mikelehen 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

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen commented Aug 5, 2019

wilhuff commented Aug 5, 2019

ghost 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

mikelehen 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

mikelehen left a comment

Choose a reason for hiding this comment

Feiyang1 left a comment

Choose a reason for hiding this comment