Enable no-floating-promises lint check and fix / suppress violations. #1087

mikelehen · 2018-08-03T15:29:26Z

I noticed that we were failing to wait for promises in a couple places so I enabled no-floating-promises lint check and cleaned up all the violations. To make this a bit easier I introduced enqueue() vs enqueueAndWait() on the AsyncQueue, with the former returning void instead of a Promise.

wilhuff · 2018-08-03T15:39:25Z

packages/firestore/src/util/async_queue.ts

  /**
   * Adds a new operation to the queue. Returns a promise that will be resolved
   * when the promise returned by the new operation is (with its value).
   */
-  enqueue<T extends Unknown>(op: () => Promise<T>): Promise<T> {
+  enqueueAndWait<T extends Unknown>(op: () => Promise<T>): Promise<T> {


The common case (and thus most likely to have a parallel on the other platforms) would be to enqueue without dropping the promise, no?

If so I'd prefer we continue to name this enqueue and call the somewhat danergous enqueue and drop promise something else.

Actually there is no enqueueAndWait() on the other platforms (dispatchAsync on iOS and enqueue on Android both return void and don't block). Instead you have to monkey with completions / TaskCompletionSource to plumb the async operation result back to the caller.

enableNetwork on web:

enableNetwork(): Promise<void> { return this.asyncQueue.enqueueAndWait(() => { return this.remoteStore.enableNetwork(); }); }

enableNetwork on iOS:

- (void)enableNetworkWithCompletion:(nullable FSTVoidErrorBlock)completion { [self.workerDispatchQueue dispatchAsync:^{ [self.remoteStore enableNetwork]; if (completion) { self->_userExecutor->Execute([=] { completion(nil); }); } }]; }

enableNetwork on Android:

public Task<Void> enableNetwork() { final TaskCompletionSource<Void> source = new TaskCompletionSource<>(); asyncQueue.enqueue( () -> { remoteStore.enableNetwork(); source.setResult(null); }); return source.getTask(); }

So I think enqueue() is arguably the common case and is named consistently with the other platforms.

Also, I can't think of a good name if I make enqueue() the "blocking" one. enqueueAndForget()?

I guess I consider enqueue-that-returns-a-promise on web to be equivalent to enqueue-that-plumbs-through-callbacks on iOS and equivalent to enqueue-with-a-manually-wrapping TaskCompletionSource on Android.

We could have Android's enqueue do the TaskCompletionSource monkeying as well (TaskCompletionSource being the equivalent of a Promise), which would bring it further into line with web.

In any case, I think we generally want to tie actions enqueued with a user-visible promise so this should be be the shorter, more obvious name.

Regarding naming: enqueueAndWait suggests actually waiting, which isn't what's happening. Meanwhile enqueueAndForget seems to reasonably suggest what it's doing to me.

Fair enough. Renamed!

I'll take a look at the Android change.

…ueue()

wilhuff

LGTM

wilhuff · 2018-08-03T19:05:20Z

packages/firestore/src/core/firestore_client.ts

@@ -435,7 +435,9 @@ export class FirestoreClient {

  write(mutations: Mutation[]): Promise<void> {
    const deferred = new Deferred<void>();
-    this.asyncQueue.enqueue(() => this.syncEngine.write(mutations, deferred));
+    this.asyncQueue.enqueueAndForget(() =>
+      this.syncEngine.write(mutations, deferred)


Could syncEngine hide this Deferred?

Not quite. The spec tests need to be able to wait for the write() to complete locally without waiting for the user callback (after the server ack of the write). So there are two separate async things you might want to wait on.

schmidt-sebastian · 2018-08-03T19:54:57Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -396,6 +396,7 @@ export class IndexedDbPersistence implements Persistence {

        // Attempt graceful shutdown (including releasing our owner lease), but
        // there's no guarantee it will complete.
+        // tslint:disable-next-line:no-floating-promises
        this.shutdown();


This runs on the queue in the multi-tab branch. Should we change it here to also use enqueueAndForget?

This code is somewhat untested and I am a little afraid of its raciness if we don't enqueue it.

I tried but we don't have access to the async queue here right now. Maybe you should get around to merging multi-tab. 😛

schmidt-sebastian · 2018-08-03T19:56:35Z

packages/firestore/test/integration/remote/stream.test.ts

              new FirestoreError(Code.UNAUTHENTICATED, '')
            );
+          })


Is there a reason you added the extra .then() instead of using await?

Yes. If I changed this then() callback to async/await then I'd feel compelled to convert the whole test, and then I'd feel compelled to convert the whole file, and at that point I may as well go ahead and convert the rest of our codebase. At that point I'd be so good at it I might as well start tackling other open source projects. And then I'd never get anything done.

mikelehen

I'm disinclined to acquiesce to your requests. Thanks anyway!

mikelehen · 2018-08-03T20:31:54Z

packages/firestore/src/core/firestore_client.ts

@@ -435,7 +435,9 @@ export class FirestoreClient {

  write(mutations: Mutation[]): Promise<void> {
    const deferred = new Deferred<void>();
-    this.asyncQueue.enqueue(() => this.syncEngine.write(mutations, deferred));
+    this.asyncQueue.enqueueAndForget(() =>
+      this.syncEngine.write(mutations, deferred)


Not quite. The spec tests need to be able to wait for the write() to complete locally without waiting for the user callback (after the server ack of the write). So there are two separate async things you might want to wait on.

mikelehen · 2018-08-03T20:33:06Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -396,6 +396,7 @@ export class IndexedDbPersistence implements Persistence {

        // Attempt graceful shutdown (including releasing our owner lease), but
        // there's no guarantee it will complete.
+        // tslint:disable-next-line:no-floating-promises
        this.shutdown();


I tried but we don't have access to the async queue here right now. Maybe you should get around to merging multi-tab. 😛

mikelehen · 2018-08-03T20:35:05Z

packages/firestore/test/integration/remote/stream.test.ts

              new FirestoreError(Code.UNAUTHENTICATED, '')
            );
+          })


Yes. If I changed this then() callback to async/await then I'd feel compelled to convert the whole test, and then I'd feel compelled to convert the whole file, and at that point I may as well go ahead and convert the rest of our codebase. At that point I'd be so good at it I might as well start tackling other open source projects. And then I'd never get anything done.

Enable no-floating-promises lint check and fix / suppress violations.

8e27955

mikelehen added the api: firestore label Aug 3, 2018

mikelehen assigned schmidt-sebastian Aug 3, 2018

mikelehen requested a review from schmidt-sebastian August 3, 2018 15:29

mikelehen requested a review from wilhuff as a code owner August 3, 2018 15:29

google-oss-bot added the needs-triage label Aug 3, 2018

wilhuff reviewed Aug 3, 2018

View reviewed changes

CR feedback: enqueue() => enqueueAndForget(), enqueueAndWait() => enq…

d1f86ca

…ueue()

wilhuff approved these changes Aug 3, 2018

View reviewed changes

schmidt-sebastian approved these changes Aug 3, 2018

View reviewed changes

mikelehen commented Aug 3, 2018

View reviewed changes

mikelehen merged commit dc9a5d1 into master Aug 3, 2018

mikelehen deleted the mikelehen/no-floating-promises branch August 3, 2018 20:37

firebase locked and limited conversation to collaborators Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable no-floating-promises lint check and fix / suppress violations. #1087

Enable no-floating-promises lint check and fix / suppress violations. #1087

Uh oh!

mikelehen commented Aug 3, 2018

Uh oh!

wilhuff Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

wilhuff Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

wilhuff left a comment

Uh oh!

wilhuff Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

schmidt-sebastian Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

schmidt-sebastian Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

mikelehen left a comment

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

mikelehen Aug 3, 2018

Uh oh!

Uh oh!

Enable no-floating-promises lint check and fix / suppress violations. #1087

Enable no-floating-promises lint check and fix / suppress violations. #1087

Uh oh!

Conversation

mikelehen commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!