Skip to content

Commit dc9a5d1

Browse files
mikelehenmikelehen
authored
Enable no-floating-promises lint check and fix / suppress violations. (#1087)
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 enqueueAndForget() on the AsyncQueue, with the latter returning void instead of a Promise.
1 parent b4ab896 commit dc9a5d1

14 files changed

+82
-64
lines changed

‎packages/firestore/src/api/database.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,8 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
305305

306306
ensureClientConfigured(): FirestoreClient {
307307
if (!this._firestoreClient) {
308+
// Kick off starting the client but don't actually wait for it.
309+
// tslint:disable-next-line:no-floating-promises
308310
this.configureClient(/* persistence= */ false);
309311
}
310312
return this._firestoreClient as FirestoreClient;

‎packages/firestore/src/core/firestore_client.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,14 @@ export class FirestoreClient {
157157
.then(() => this.initializeRest(user))
158158
.then(initializationDone.resolve, initializationDone.reject);
159159
} else {
160-
this.asyncQueue.enqueue(() => {
160+
this.asyncQueue.enqueueAndForget(() => {
161161
return this.handleUserChange(user);
162162
});
163163
}
164164
});
165165

166166
// Block the async queue until initialization is done
167-
this.asyncQueue.enqueue(() => {
167+
this.asyncQueue.enqueueAndForget(() => {
168168
return initializationDone.promise;
169169
});
170170

@@ -385,14 +385,14 @@ export class FirestoreClient {
385385
options: ListenOptions
386386
): QueryListener {
387387
const listener = new QueryListener(query, observer, options);
388-
this.asyncQueue.enqueue(() => {
388+
this.asyncQueue.enqueueAndForget(() => {
389389
return this.eventMgr.listen(listener);
390390
});
391391
return listener;
392392
}
393393

394394
unlisten(listener: QueryListener): void {
395-
this.asyncQueue.enqueue(() => {
395+
this.asyncQueue.enqueueAndForget(() => {
396396
return this.eventMgr.unlisten(listener);
397397
});
398398
}
@@ -435,7 +435,9 @@ export class FirestoreClient {
435435

436436
write(mutations: Mutation[]): Promise<void> {
437437
const deferred = new Deferred<void>();
438-
this.asyncQueue.enqueue(() => this.syncEngine.write(mutations, deferred));
438+
this.asyncQueue.enqueueAndForget(() =>
439+
this.syncEngine.write(mutations, deferred)
440+
);
439441
return deferred.promise;
440442
}
441443

‎packages/firestore/src/local/indexeddb_persistence.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,7 @@ export class IndexedDbPersistence implements Persistence {
396396

397397
// Attempt graceful shutdown (including releasing our owner lease), but
398398
// there's no guarantee it will complete.
399+
// tslint:disable-next-line:no-floating-promises
399400
this.shutdown();
400401
};
401402
window.addEventListener('unload', this.windowUnloadHandler);

‎packages/firestore/src/remote/persistent_stream.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,9 +241,9 @@ export abstract class PersistentStream<
241241
*
242242
* When stop returns, isStarted() and isOpen() will both return false.
243243
*/
244-
stop(): void {
244+
async stop(): Promise<void> {
245245
if (this.isStarted()) {
246-
this.close(PersistentStreamState.Initial);
246+
await this.close(PersistentStreamState.Initial);
247247
}
248248
}
249249

@@ -502,7 +502,7 @@ export abstract class PersistentStream<
502502
startCloseCount: number
503503
): (fn: () => Promise<void>) => void {
504504
return (fn: () => Promise<void>): void => {
505-
this.queue.enqueue(() => {
505+
this.queue.enqueueAndForget(() => {
506506
if (this.closeCount === startCloseCount) {
507507
return fn();
508508
} else {

‎packages/firestore/src/remote/remote_store.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,8 @@ export class RemoteStore implements TargetMetadataProvider {
188188
if (this.networkEnabled) {
189189
this.networkEnabled = false;
190190

191-
this.writeStream.stop();
192-
this.watchStream.stop();
191+
await this.writeStream.stop();
192+
await this.watchStream.stop();
193193

194194
if (this.writePipeline.length > 0) {
195195
log.debug(
@@ -205,14 +205,13 @@ export class RemoteStore implements TargetMetadataProvider {
205205
}
206206
}
207207

208-
shutdown(): Promise<void> {
208+
async shutdown(): Promise<void> {
209209
log.debug(LOG_TAG, 'RemoteStore shutting down.');
210-
this.disableNetworkInternal();
210+
await this.disableNetworkInternal();
211211

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

218217
/** Starts new listen for the given query. Uses resume token if provided */
@@ -671,7 +670,7 @@ export class RemoteStore implements TargetMetadataProvider {
671670
// Tear down and re-create our network streams. This will ensure we get a fresh auth token
672671
// for the new user and re-fill the write pipeline with new mutations from the LocalStore
673672
// (since mutations are per-user).
674-
this.disableNetworkInternal();
673+
await this.disableNetworkInternal();
675674
this.onlineStateTracker.set(OnlineState.Unknown);
676675
await this.enableNetwork();
677676
}

‎packages/firestore/src/util/async_queue.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ class DelayedOperation<T extends Unknown> implements CancelablePromise<T> {
154154
catch = this.deferred.promise.catch.bind(this.deferred.promise);
155155

156156
private handleDelayElapsed(): void {
157-
this.asyncQueue.enqueue(() => {
157+
this.asyncQueue.enqueueAndForget(() => {
158158
if (this.timerHandle !== null) {
159159
this.clearTimeout();
160160
return this.op().then(result => {
@@ -190,6 +190,15 @@ export class AsyncQueue {
190190
// assertion sanity-checks.
191191
private operationInProgress = false;
192192

193+
/**
194+
* Adds a new operation to the queue without waiting for it to complete (i.e.
195+
* we ignore the Promise result).
196+
*/
197+
enqueueAndForget<T extends Unknown>(op: () => Promise<T>): void {
198+
// tslint:disable-next-line:no-floating-promises
199+
this.enqueue(op);
200+
}
201+
193202
/**
194203
* Adds a new operation to the queue. Returns a promise that will be resolved
195204
* when the promise returned by the new operation is (with its value).

‎packages/firestore/test/integration/api/array_transforms.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ apiDescribe('Array Transforms:', persistence => {
8484
it('create document with arrayUnion()', async () => {
8585
await withTestSetup(async () => {
8686
await docRef.set({ array: FieldValue.arrayUnion(1, 2) });
87-
expectLocalAndRemoteEvent({ array: [1, 2] });
87+
await expectLocalAndRemoteEvent({ array: [1, 2] });
8888
});
8989
});
9090

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

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,10 @@ describe('Watch Stream', () => {
144144
watchStream = ds.newPersistentWatchStream(streamListener);
145145
watchStream.start();
146146

147-
return streamListener.awaitCallback('open').then(() => {
148-
watchStream.stop();
147+
return streamListener.awaitCallback('open').then(async () => {
148+
await watchStream.stop();
149149

150-
return streamListener.awaitCallback('close');
150+
await streamListener.awaitCallback('close');
151151
});
152152
});
153153
});
@@ -193,10 +193,10 @@ describe('Write Stream', () => {
193193
writeStream = ds.newPersistentWriteStream(streamListener);
194194
writeStream.start();
195195
return streamListener.awaitCallback('open');
196-
}).then(() => {
197-
writeStream.stop();
196+
}).then(async () => {
197+
await writeStream.stop();
198198

199-
return streamListener.awaitCallback('close');
199+
await streamListener.awaitCallback('close');
200200
});
201201
});
202202

@@ -221,10 +221,10 @@ describe('Write Stream', () => {
221221
writeStream.writeMutations(SINGLE_MUTATION);
222222
return streamListener.awaitCallback('mutationResult');
223223
})
224-
.then(() => {
225-
writeStream.stop();
224+
.then(async () => {
225+
await writeStream.stop();
226226

227-
return streamListener.awaitCallback('close');
227+
await streamListener.awaitCallback('close');
228228
});
229229
});
230230

@@ -295,9 +295,11 @@ describe('Write Stream', () => {
295295
.then(() => {
296296
// Simulate callback from GRPC with an unauthenticated error -- this should invalidate
297297
// the token.
298-
writeStream.handleStreamClose(
298+
return writeStream.handleStreamClose(
299299
new FirestoreError(Code.UNAUTHENTICATED, '')
300300
);
301+
})
302+
.then(() => {
301303
return streamListener.awaitCallback('close');
302304
})
303305
.then(() => {
@@ -306,9 +308,11 @@ describe('Write Stream', () => {
306308
})
307309
.then(() => {
308310
// Simulate a different error -- token should not be invalidated this time.
309-
writeStream.handleStreamClose(
311+
return writeStream.handleStreamClose(
310312
new FirestoreError(Code.UNAVAILABLE, '')
311313
);
314+
})
315+
.then(() => {
312316
return streamListener.awaitCallback('close');
313317
})
314318
.then(() => {

‎packages/firestore/test/unit/core/event_manager.test.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -57,37 +57,37 @@ describe('EventManager', () => {
5757
return stub;
5858
}
5959

60-
it('handles many listenables per query', () => {
60+
it('handles many listenables per query', async () => {
6161
const query = Query.atPath(path('foo/bar'));
6262
const fakeListener1 = fakeQueryListener(query);
6363
const fakeListener2 = fakeQueryListener(query);
6464

6565
const syncEngineSpy = makeSyncEngineSpy();
6666
const eventManager = new EventManager(syncEngineSpy);
6767

68-
eventManager.listen(fakeListener1);
68+
await eventManager.listen(fakeListener1);
6969
expect(syncEngineSpy.listen.calledWith(query)).to.be.true;
7070

71-
eventManager.listen(fakeListener2);
71+
await eventManager.listen(fakeListener2);
7272
expect(syncEngineSpy.listen.callCount).to.equal(1);
7373

74-
eventManager.unlisten(fakeListener2);
74+
await eventManager.unlisten(fakeListener2);
7575
expect(syncEngineSpy.unlisten.callCount).to.equal(0);
7676

77-
eventManager.unlisten(fakeListener1);
77+
await eventManager.unlisten(fakeListener1);
7878
expect(syncEngineSpy.unlisten.calledWith(query)).to.be.true;
7979
});
8080

81-
it('handles unlisten on unknown listenable gracefully', () => {
81+
it('handles unlisten on unknown listenable gracefully', async () => {
8282
const syncEngineSpy = makeSyncEngineSpy();
8383
const query = Query.atPath(path('foo/bar'));
8484
const fakeListener1 = fakeQueryListener(query);
8585
const eventManager = new EventManager(syncEngineSpy);
86-
eventManager.unlisten(fakeListener1);
86+
await eventManager.unlisten(fakeListener1);
8787
expect(syncEngineSpy.unlisten.callCount).to.equal(0);
8888
});
8989

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

111-
eventManager.listen(fakeListener1);
112-
eventManager.listen(fakeListener2);
113-
eventManager.listen(fakeListener3);
111+
await eventManager.listen(fakeListener1);
112+
await eventManager.listen(fakeListener2);
113+
await eventManager.listen(fakeListener3);
114114
expect(syncEngineSpy.listen.callCount).to.equal(2);
115115

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

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

140-
eventManager.listen(fakeListener1);
140+
await eventManager.listen(fakeListener1);
141141
expect(events).to.deep.equal([OnlineState.Unknown]);
142142
eventManager.applyOnlineStateChange(OnlineState.Online);
143143
expect(events).to.deep.equal([OnlineState.Unknown, OnlineState.Online]);

‎packages/firestore/test/unit/local/mutation_queue.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,12 +220,12 @@ function genericMutationQueueTests(): void {
220220
BATCHID_UNKNOWN
221221
);
222222

223-
mutationQueue.acknowledgeBatch(batch1, emptyByteString());
223+
await mutationQueue.acknowledgeBatch(batch1, emptyByteString());
224224
expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal(
225225
batch1.batchId
226226
);
227227

228-
mutationQueue.acknowledgeBatch(batch2, emptyByteString());
228+
await mutationQueue.acknowledgeBatch(batch2, emptyByteString());
229229
expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal(
230230
batch2.batchId
231231
);

‎packages/firestore/test/unit/local/query_cache.test.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ function genericQueryCacheTests(
9797
});
9898

9999
afterEach(async () => {
100-
persistence.shutdown(/* deleteData= */ true);
100+
await persistence.shutdown(/* deleteData= */ true);
101101
});
102102

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

208-
cache.addMatchingKeys([key1, key2], 1);
209-
cache.addMatchingKeys([key3], 2);
208+
await cache.addMatchingKeys([key1, key2], 1);
209+
await cache.addMatchingKeys([key3], 2);
210210
expect(await cache.containsKey(key1)).to.equal(true);
211211
expect(await cache.containsKey(key2)).to.equal(true);
212212
expect(await cache.containsKey(key3)).to.equal(true);
213213

214-
cache.removeMatchingKeysForTargetId(1);
214+
await cache.removeMatchingKeysForTargetId(1);
215215
expect(await cache.containsKey(key1)).to.equal(false);
216216
expect(await cache.containsKey(key2)).to.equal(false);
217217
expect(await cache.containsKey(key3)).to.equal(true);
218218

219-
cache.removeMatchingKeysForTargetId(2);
219+
await cache.removeMatchingKeysForTargetId(2);
220220
expect(await cache.containsKey(key1)).to.equal(false);
221221
expect(await cache.containsKey(key2)).to.equal(false);
222222
expect(await cache.containsKey(key3)).to.equal(false);
@@ -244,13 +244,13 @@ function genericQueryCacheTests(
244244

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

247-
cache.removeMatchingKeys([room1], rooms.targetId);
247+
await cache.removeMatchingKeys([room1], rooms.targetId);
248248
expect(await testGc.collectGarbage()).to.deep.equal([room1]);
249249

250-
cache.removeQueryData(rooms);
250+
await cache.removeQueryData(rooms);
251251
expect(await testGc.collectGarbage()).to.deep.equal([room2]);
252252

253-
cache.removeMatchingKeysForTargetId(halls.targetId);
253+
await cache.removeMatchingKeysForTargetId(halls.targetId);
254254
expect(await testGc.collectGarbage()).to.deep.equal([hall1, hall2]);
255255
});
256256

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

271-
cache.addMatchingKeys([key1], 2);
271+
await cache.addMatchingKeys([key1], 2);
272272
expect(await cache.getMatchingKeysForTargetId(1)).to.deep.equal([
273273
key1,
274274
key2

‎packages/firestore/test/unit/specs/spec_test_runner.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ class MockConnection implements Connection {
237237
this.resetAndCloseWriteStream();
238238
}
239239
});
240-
this.queue.enqueue(async () => {
240+
this.queue.enqueueAndForget(async () => {
241241
if (this.writeStream === writeStream) {
242242
writeStream.callOnOpen();
243243
}
@@ -270,7 +270,7 @@ class MockConnection implements Connection {
270270
}
271271
});
272272
// Call on open immediately after returning
273-
this.queue.enqueue(async () => {
273+
this.queue.enqueueAndForget(async () => {
274274
if (this.watchStream === watchStream) {
275275
watchStream.callOnOpen();
276276
this.watchOpen.resolve();

0 commit comments

Comments
 (0)