Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 3, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e27955
Enable no-floating-promises lint check and fix / suppress violations.
Aug 3, 2018
d1f86ca
CR feedback: enqueue() => enqueueAndForget(), enqueueAndWait() => enq…
Aug 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,8 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {

ensureClientConfigured(): FirestoreClient {
if (!this._firestoreClient) {
// Kick off starting the client but don't actually wait for it.
// tslint:disable-next-line:no-floating-promises
this.configureClient(/* persistence= */ false);
}
return this._firestoreClient as FirestoreClient;
Expand Down
12 changes: 7 additions & 5 deletions packages/firestore/src/core/firestore_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,14 @@ export class FirestoreClient {
.then(() => this.initializeRest(user))
.then(initializationDone.resolve, initializationDone.reject);
} else {
this.asyncQueue.enqueue(() => {
this.asyncQueue.enqueueAndForget(() => {
return this.handleUserChange(user);
});
}
});

// Block the async queue until initialization is done
this.asyncQueue.enqueue(() => {
this.asyncQueue.enqueueAndForget(() => {
return initializationDone.promise;
});

Expand Down Expand Up @@ -385,14 +385,14 @@ export class FirestoreClient {
options: ListenOptions
): QueryListener {
const listener = new QueryListener(query, observer, options);
this.asyncQueue.enqueue(() => {
this.asyncQueue.enqueueAndForget(() => {
return this.eventMgr.listen(listener);
});
return listener;
}

unlisten(listener: QueryListener): void {
this.asyncQueue.enqueue(() => {
this.asyncQueue.enqueueAndForget(() => {
return this.eventMgr.unlisten(listener);
});
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could syncEngine hide this Deferred?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
return deferred.promise;
}

Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/local/indexeddb_persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
window.addEventListener('unload', this.windowUnloadHandler);
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/src/remote/persistent_stream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,9 @@ export abstract class PersistentStream<
*
* When stop returns, isStarted() and isOpen() will both return false.
*/
stop(): void {
async stop(): Promise<void> {
if (this.isStarted()) {
this.close(PersistentStreamState.Initial);
await this.close(PersistentStreamState.Initial);
}
}

Expand Down Expand Up @@ -502,7 +502,7 @@ export abstract class PersistentStream<
startCloseCount: number
): (fn: () => Promise<void>) => void {
return (fn: () => Promise<void>): void => {
this.queue.enqueue(() => {
this.queue.enqueueAndForget(() => {
if (this.closeCount === startCloseCount) {
return fn();
} else {
Expand Down
11 changes: 5 additions & 6 deletions packages/firestore/src/remote/remote_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ export class RemoteStore implements TargetMetadataProvider {
if (this.networkEnabled) {
this.networkEnabled = false;

this.writeStream.stop();
this.watchStream.stop();
await this.writeStream.stop();
await this.watchStream.stop();

if (this.writePipeline.length > 0) {
log.debug(
Expand All @@ -205,14 +205,13 @@ export class RemoteStore implements TargetMetadataProvider {
}
}

shutdown(): Promise<void> {
async shutdown(): Promise<void> {
log.debug(LOG_TAG, 'RemoteStore shutting down.');
this.disableNetworkInternal();
await this.disableNetworkInternal();

// Set the OnlineState to Unknown (rather than Offline) to avoid potentially
// triggering spurious listener events with cached data, etc.
this.onlineStateTracker.set(OnlineState.Unknown);
return Promise.resolve();
}

/** Starts new listen for the given query. Uses resume token if provided */
Expand Down Expand Up @@ -671,7 +670,7 @@ export class RemoteStore implements TargetMetadataProvider {
// Tear down and re-create our network streams. This will ensure we get a fresh auth token
// for the new user and re-fill the write pipeline with new mutations from the LocalStore
// (since mutations are per-user).
this.disableNetworkInternal();
await this.disableNetworkInternal();
this.onlineStateTracker.set(OnlineState.Unknown);
await this.enableNetwork();
}
Expand Down
11 changes: 10 additions & 1 deletion packages/firestore/src/util/async_queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class DelayedOperation<T extends Unknown> implements CancelablePromise<T> {
catch = this.deferred.promise.catch.bind(this.deferred.promise);

private handleDelayElapsed(): void {
this.asyncQueue.enqueue(() => {
this.asyncQueue.enqueueAndForget(() => {
if (this.timerHandle !== null) {
this.clearTimeout();
return this.op().then(result => {
Expand Down Expand Up @@ -190,6 +190,15 @@ export class AsyncQueue {
// assertion sanity-checks.
private operationInProgress = false;

/**
* Adds a new operation to the queue without waiting for it to complete (i.e.
* we ignore the Promise result).
*/
enqueueAndForget<T extends Unknown>(op: () => Promise<T>): void {
// tslint:disable-next-line:no-floating-promises
this.enqueue(op);
}

/**
* 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).
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/integration/api/array_transforms.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ apiDescribe('Array Transforms:', persistence => {
it('create document with arrayUnion()', async () => {
await withTestSetup(async () => {
await docRef.set({ array: FieldValue.arrayUnion(1, 2) });
expectLocalAndRemoteEvent({ array: [1, 2] });
await expectLocalAndRemoteEvent({ array: [1, 2] });
});
});

Expand Down
26 changes: 15 additions & 11 deletions packages/firestore/test/integration/remote/stream.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,10 @@ describe('Watch Stream', () => {
watchStream = ds.newPersistentWatchStream(streamListener);
watchStream.start();

return streamListener.awaitCallback('open').then(() => {
watchStream.stop();
return streamListener.awaitCallback('open').then(async () => {
await watchStream.stop();

return streamListener.awaitCallback('close');
await streamListener.awaitCallback('close');
});
});
});
Expand Down Expand Up @@ -193,10 +193,10 @@ describe('Write Stream', () => {
writeStream = ds.newPersistentWriteStream(streamListener);
writeStream.start();
return streamListener.awaitCallback('open');
}).then(() => {
writeStream.stop();
}).then(async () => {
await writeStream.stop();

return streamListener.awaitCallback('close');
await streamListener.awaitCallback('close');
});
});

Expand All @@ -221,10 +221,10 @@ describe('Write Stream', () => {
writeStream.writeMutations(SINGLE_MUTATION);
return streamListener.awaitCallback('mutationResult');
})
.then(() => {
writeStream.stop();
.then(async () => {
await writeStream.stop();

return streamListener.awaitCallback('close');
await streamListener.awaitCallback('close');
});
});

Expand Down Expand Up @@ -295,9 +295,11 @@ describe('Write Stream', () => {
.then(() => {
// Simulate callback from GRPC with an unauthenticated error -- this should invalidate
// the token.
writeStream.handleStreamClose(
return writeStream.handleStreamClose(
new FirestoreError(Code.UNAUTHENTICATED, '')
);
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.then(() => {
return streamListener.awaitCallback('close');
})
.then(() => {
Expand All @@ -306,9 +308,11 @@ describe('Write Stream', () => {
})
.then(() => {
// Simulate a different error -- token should not be invalidated this time.
writeStream.handleStreamClose(
return writeStream.handleStreamClose(
new FirestoreError(Code.UNAVAILABLE, '')
);
})
.then(() => {
return streamListener.awaitCallback('close');
})
.then(() => {
Expand Down
26 changes: 13 additions & 13 deletions packages/firestore/test/unit/core/event_manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,37 +57,37 @@ describe('EventManager', () => {
return stub;
}

it('handles many listenables per query', () => {
it('handles many listenables per query', async () => {
const query = Query.atPath(path('foo/bar'));
const fakeListener1 = fakeQueryListener(query);
const fakeListener2 = fakeQueryListener(query);

const syncEngineSpy = makeSyncEngineSpy();
const eventManager = new EventManager(syncEngineSpy);

eventManager.listen(fakeListener1);
await eventManager.listen(fakeListener1);
expect(syncEngineSpy.listen.calledWith(query)).to.be.true;

eventManager.listen(fakeListener2);
await eventManager.listen(fakeListener2);
expect(syncEngineSpy.listen.callCount).to.equal(1);

eventManager.unlisten(fakeListener2);
await eventManager.unlisten(fakeListener2);
expect(syncEngineSpy.unlisten.callCount).to.equal(0);

eventManager.unlisten(fakeListener1);
await eventManager.unlisten(fakeListener1);
expect(syncEngineSpy.unlisten.calledWith(query)).to.be.true;
});

it('handles unlisten on unknown listenable gracefully', () => {
it('handles unlisten on unknown listenable gracefully', async () => {
const syncEngineSpy = makeSyncEngineSpy();
const query = Query.atPath(path('foo/bar'));
const fakeListener1 = fakeQueryListener(query);
const eventManager = new EventManager(syncEngineSpy);
eventManager.unlisten(fakeListener1);
await eventManager.unlisten(fakeListener1);
expect(syncEngineSpy.unlisten.callCount).to.equal(0);
});

it('notifies listenables in the right order', () => {
it('notifies listenables in the right order', async () => {
const query1 = Query.atPath(path('foo/bar'));
const query2 = Query.atPath(path('bar/baz'));
const eventOrder: string[] = [];
Expand All @@ -108,9 +108,9 @@ describe('EventManager', () => {
const syncEngineSpy = makeSyncEngineSpy();
const eventManager = new EventManager(syncEngineSpy);

eventManager.listen(fakeListener1);
eventManager.listen(fakeListener2);
eventManager.listen(fakeListener3);
await eventManager.listen(fakeListener1);
await eventManager.listen(fakeListener2);
await eventManager.listen(fakeListener3);
expect(syncEngineSpy.listen.callCount).to.equal(2);

// tslint:disable-next-line:no-any mock ViewSnapshot.
Expand All @@ -126,7 +126,7 @@ describe('EventManager', () => {
]);
});

it('will forward applyOnlineStateChange calls', () => {
it('will forward applyOnlineStateChange calls', async () => {
const query = Query.atPath(path('foo/bar'));
const fakeListener1 = fakeQueryListener(query);
const events: OnlineState[] = [];
Expand All @@ -137,7 +137,7 @@ describe('EventManager', () => {
const syncEngineSpy = makeSyncEngineSpy();
const eventManager = new EventManager(syncEngineSpy);

eventManager.listen(fakeListener1);
await eventManager.listen(fakeListener1);
expect(events).to.deep.equal([OnlineState.Unknown]);
eventManager.applyOnlineStateChange(OnlineState.Online);
expect(events).to.deep.equal([OnlineState.Unknown, OnlineState.Online]);
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/test/unit/local/mutation_queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,12 @@ function genericMutationQueueTests(): void {
BATCHID_UNKNOWN
);

mutationQueue.acknowledgeBatch(batch1, emptyByteString());
await mutationQueue.acknowledgeBatch(batch1, emptyByteString());
expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal(
batch1.batchId
);

mutationQueue.acknowledgeBatch(batch2, emptyByteString());
await mutationQueue.acknowledgeBatch(batch2, emptyByteString());
expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal(
batch2.batchId
);
Expand Down
18 changes: 9 additions & 9 deletions packages/firestore/test/unit/local/query_cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ function genericQueryCacheTests(
});

afterEach(async () => {
persistence.shutdown(/* deleteData= */ true);
await persistence.shutdown(/* deleteData= */ true);
});

it('returns null for query not in cache', () => {
Expand Down Expand Up @@ -205,18 +205,18 @@ function genericQueryCacheTests(
const key2 = key('foo/baz');
const key3 = key('foo/blah');

cache.addMatchingKeys([key1, key2], 1);
cache.addMatchingKeys([key3], 2);
await cache.addMatchingKeys([key1, key2], 1);
await cache.addMatchingKeys([key3], 2);
expect(await cache.containsKey(key1)).to.equal(true);
expect(await cache.containsKey(key2)).to.equal(true);
expect(await cache.containsKey(key3)).to.equal(true);

cache.removeMatchingKeysForTargetId(1);
await cache.removeMatchingKeysForTargetId(1);
expect(await cache.containsKey(key1)).to.equal(false);
expect(await cache.containsKey(key2)).to.equal(false);
expect(await cache.containsKey(key3)).to.equal(true);

cache.removeMatchingKeysForTargetId(2);
await cache.removeMatchingKeysForTargetId(2);
expect(await cache.containsKey(key1)).to.equal(false);
expect(await cache.containsKey(key2)).to.equal(false);
expect(await cache.containsKey(key3)).to.equal(false);
Expand Down Expand Up @@ -244,13 +244,13 @@ function genericQueryCacheTests(

expect(await testGc.collectGarbage()).to.deep.equal([]);

cache.removeMatchingKeys([room1], rooms.targetId);
await cache.removeMatchingKeys([room1], rooms.targetId);
expect(await testGc.collectGarbage()).to.deep.equal([room1]);

cache.removeQueryData(rooms);
await cache.removeQueryData(rooms);
expect(await testGc.collectGarbage()).to.deep.equal([room2]);

cache.removeMatchingKeysForTargetId(halls.targetId);
await cache.removeMatchingKeysForTargetId(halls.targetId);
expect(await testGc.collectGarbage()).to.deep.equal([hall1, hall2]);
});

Expand All @@ -268,7 +268,7 @@ function genericQueryCacheTests(
]);
expect(await cache.getMatchingKeysForTargetId(2)).to.deep.equal([key3]);

cache.addMatchingKeys([key1], 2);
await cache.addMatchingKeys([key1], 2);
expect(await cache.getMatchingKeysForTargetId(1)).to.deep.equal([
key1,
key2
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ class MockConnection implements Connection {
this.resetAndCloseWriteStream();
}
});
this.queue.enqueue(async () => {
this.queue.enqueueAndForget(async () => {
if (this.writeStream === writeStream) {
writeStream.callOnOpen();
}
Expand Down Expand Up @@ -270,7 +270,7 @@ class MockConnection implements Connection {
}
});
// Call on open immediately after returning
this.queue.enqueue(async () => {
this.queue.enqueueAndForget(async () => {
if (this.watchStream === watchStream) {
watchStream.callOnOpen();
this.watchOpen.resolve();
Expand Down
Loading