From 454e2f3a67dfe4f09bc47af32ce807128b6c4fec Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Sep 2020 12:25:53 -0700 Subject: [PATCH 1/4] Support waitForPendingWrites in secondary tabs --- packages/firestore/src/core/sync_engine.ts | 1 + .../firestore/test/unit/specs/spec_builder.ts | 15 +++ .../test/unit/specs/spec_test_runner.ts | 38 +++++- .../test/unit/specs/write_spec.test.ts | 113 ++++++++++++++++++ 4 files changed, 166 insertions(+), 1 deletion(-) diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index c0c6a27526f..897ecbe0dea 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -1185,6 +1185,7 @@ export async function applyBatchState( // NOTE: Both these methods are no-ops for batches that originated from // other clients. processUserCallback(syncEngineImpl, batchId, error ? error : null); + triggerPendingWritesCallbacks(syncEngineImpl, batchId); removeCachedMutationBatchMetadata(syncEngineImpl.localStore, batchId); } else { fail(`Unknown batchState: ${batchState}`); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 1481de1cc47..e370ca194a9 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -793,6 +793,14 @@ export class SpecBuilder { return this; } + waitForPendingWrites(): this { + this.nextStep(); + this.currentStep = { + waitForPendingWrites: true + }; + return this; + } + expectUserCallbacks(docs: { acknowledged?: string[]; rejected?: string[]; @@ -944,6 +952,13 @@ export class SpecBuilder { return this; } + expectWaitForPendingWritesEvent(count = 1): this { + this.assertStep('Expectations require previous step'); + const currentStep = this.currentStep!; + currentStep.expectedWaitForPendingWritesEvents = count; + return this; + } + private static queryToSpec(query: Query): SpecQuery { // TODO(dimond): full query support const spec: SpecQuery = { path: query.path.canonicalString() }; diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index a9dd56b66d9..5f474c466d8 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -44,6 +44,7 @@ import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { activeLimboDocumentResolutions, enqueuedLimboDocumentResolutions, + registerPendingWritesCallback, SyncEngine, syncEngineListen, syncEngineUnlisten, @@ -195,6 +196,7 @@ abstract class TestRunner { private eventList: QueryEvent[] = []; private acknowledgedDocs: string[]; private rejectedDocs: string[]; + private waitForPendingWritesEvents = 0; private snapshotsInSyncListeners: Array>; private snapshotsInSyncEvents = 0; @@ -342,6 +344,9 @@ abstract class TestRunner { this.validateExpectedSnapshotEvents(step.expectedSnapshotEvents!); await this.validateExpectedState(step.expectedState!); this.validateSnapshotsInSyncEvents(step.expectedSnapshotsInSyncEvents); + this.validateWaitForPendingWritesEvents( + step.expectedWaitForPendingWritesEvents + ); this.eventList = []; this.rejectedDocs = []; this.acknowledgedDocs = []; @@ -382,6 +387,8 @@ abstract class TestRunner { return this.doWriteAck(step.writeAck!); } else if ('failWrite' in step) { return this.doFailWrite(step.failWrite!); + } else if ('waitForPendingWrites' in step) { + return this.doWaitForPendingWrites(); } else if ('runTimer' in step) { return this.doRunTimer(step.runTimer!); } else if ('drainQueue' in step) { @@ -716,6 +723,14 @@ abstract class TestRunner { }); } + private async doWaitForPendingWrites(): Promise { + const deferred = new Deferred(); + void deferred.promise.then(() => ++this.waitForPendingWritesEvents); + return this.queue.enqueue(() => + registerPendingWritesCallback(this.syncEngine, deferred) + ); + } + private async doRunTimer(timer: string): Promise { // We assume the timer string is a valid TimerID enum value, but if it's // not, then there won't be a matching item on the queue and @@ -912,10 +927,23 @@ abstract class TestRunner { } } + private validateWaitForPendingWritesEvents( + expectedCount: number | undefined + ): void { + expect(this.waitForPendingWritesEvents).to.eq( + expectedCount || 0, + 'for waitForPendingWritesEvents' + ); + this.waitForPendingWritesEvents = 0; + } + private validateSnapshotsInSyncEvents( expectedCount: number | undefined ): void { - expect(this.snapshotsInSyncEvents).to.eq(expectedCount || 0); + expect(this.snapshotsInSyncEvents).to.eq( + expectedCount || 0, + 'for snapshotsInSyncEvents' + ); this.snapshotsInSyncEvents = 0; } @@ -1334,6 +1362,8 @@ export interface SpecStep { writeAck?: SpecWriteAck; /** Fail a write */ failWrite?: SpecWriteFailure; + /** Add a new `waitForPendingWrites` listener. */ + waitForPendingWrites?: true; /** Fails the listed database actions. */ failDatabase?: false | PersistenceAction[]; @@ -1387,6 +1417,12 @@ export interface SpecStep { * If not provided, the test will fail if the step causes events to be raised. */ expectedSnapshotsInSyncEvents?: number; + + /** + * Optional expected number of waitForPendingWrite callbacks to be called. + * If not provided, the test will fail if the step causes events to be raised. + */ + expectedWaitForPendingWritesEvents?: number; } /** [, ] */ diff --git a/packages/firestore/test/unit/specs/write_spec.test.ts b/packages/firestore/test/unit/specs/write_spec.test.ts index f9c5e3ed514..3cb8b26c782 100644 --- a/packages/firestore/test/unit/specs/write_spec.test.ts +++ b/packages/firestore/test/unit/specs/write_spec.test.ts @@ -1490,4 +1490,117 @@ describeSpec('Writes:', [], () => { .expectEvents(query1, { added: [docA, docB], fromCache: true }); } ); + + specTest( + 'Wait for pending writes resolves after write acknowledgment', + [], + () => { + return spec() + .userSets('collection/a', { k: 'a' }) + .userSets('collection/b', { k: 'b' }) + .waitForPendingWrites() + .writeAcks('collection/a', 1001) + .failWrite( + 'collection/b', + new RpcError(Code.FAILED_PRECONDITION, 'Write error') + ) + .expectWaitForPendingWritesEvent(); + } + ); + + specTest('Wait for pending writes resolves with no writes', [], () => { + return spec().waitForPendingWrites().expectWaitForPendingWritesEvent(); + }); + + specTest('Wait for pending writes resolves multiple times', [], () => { + return spec() + .userSets('collection/a', { k: 'a' }) + .waitForPendingWrites() + .waitForPendingWrites() + .writeAcks('collection/a', 1001) + .expectWaitForPendingWritesEvent(2); + }); + + specTest( + 'Wait for pending writes resolves if another write is issued', + [], + () => { + return spec() + .userSets('collection/a', { k: 'a' }) + .waitForPendingWrites() + .userSets('collection/b', { k: 'b' }) + .writeAcks('collection/a', 1001) + .expectWaitForPendingWritesEvent() + .writeAcks('collection/b', 1002); + } + ); + + specTest( + 'Wait for pending writes waits after restart', + ['durable-persistence'], + () => { + return spec() + .userSets('collection/a', { k: 'a' }) + .restart() + .waitForPendingWrites() + .writeAcks('collection/a', 1001, { expectUserCallback: false }) + .expectWaitForPendingWritesEvent(); + } + ); + + specTest( + 'Wait for pending writes resolves for write in secondary tab', + ['multi-client'], + () => { + return client(0) + .expectPrimaryState(true) + .client(1) + .userSets('collection/a', { k: 'a' }) + .waitForPendingWrites() + .client(0) + .writeAcks('collection/a', 1001, { expectUserCallback: false }) + .client(1) + .expectUserCallbacks({ acknowledged: ['collection/a'] }) + .expectWaitForPendingWritesEvent(); + } + ); + + specTest( + 'Wait for pending writes resolves independently for different tabs', + ['multi-client'], + () => { + return client(0) + .userSets('collection/a', { k: 'a' }) + .waitForPendingWrites() + .client(1) + .userSets('collection/b', { k: 'b' }) + .waitForPendingWrites() + .client(2) + .userSets('collection/c', { k: 'c' }) + .waitForPendingWrites() + .client(0) + .writeAcks('collection/a', 1001) + .expectWaitForPendingWritesEvent(/* count= */ 1) + .client(1) + .expectWaitForPendingWritesEvent(/* count= */ 0) + .client(2) + .expectWaitForPendingWritesEvent(/* count= */ 0) + .client(0) + .writeAcks('collection/b', 1002, { expectUserCallback: false }) + .expectWaitForPendingWritesEvent(/* count= */ 0) + .client(1) + .expectUserCallbacks({ acknowledged: ['collection/b'] }) + .expectWaitForPendingWritesEvent(/* count= */ 1) + .client(2) + .expectWaitForPendingWritesEvent(/* count= */ 0) + .client(0) + .writeAcks('collection/c', 1003, { expectUserCallback: false }) + .expectWaitForPendingWritesEvent(/* count= */ 0) + .client(1) + .expectWaitForPendingWritesEvent(/* count= */ 0) + .client(2) + .expectUserCallbacks({ acknowledged: ['collection/c'] }) + .expectWaitForPendingWritesEvent(/* count= */ 1); + } + ); }); From f0b7a1d5679dadc85195fdf96bfe8e7a48879fe1 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Sep 2020 12:39:03 -0700 Subject: [PATCH 2/4] Create mean-queens-roll.md --- .changeset/mean-queens-roll.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/mean-queens-roll.md diff --git a/.changeset/mean-queens-roll.md b/.changeset/mean-queens-roll.md new file mode 100644 index 00000000000..c5d91cbdaaa --- /dev/null +++ b/.changeset/mean-queens-roll.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Fixes an issue that prevent `waitForPendingWrites()` to resolve in background tabs when multi-tab is used. From 4666776b4a36cb587f5fba1c5db2115355a4ac33 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Sep 2020 13:26:41 -0700 Subject: [PATCH 3/4] Update mean-queens-roll.md --- .changeset/mean-queens-roll.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/mean-queens-roll.md b/.changeset/mean-queens-roll.md index c5d91cbdaaa..05ccad35354 100644 --- a/.changeset/mean-queens-roll.md +++ b/.changeset/mean-queens-roll.md @@ -2,4 +2,4 @@ "@firebase/firestore": patch --- -Fixes an issue that prevent `waitForPendingWrites()` to resolve in background tabs when multi-tab is used. +Fixes an issue that prevents `waitForPendingWrites()` from resolving in background tabs when multi-tab is used. From d4adc1e93511afbb2746cf98c0a0417ec47356ee Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Sep 2020 14:58:01 -0700 Subject: [PATCH 4/4] Update mean-queens-roll.md --- .changeset/mean-queens-roll.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/mean-queens-roll.md b/.changeset/mean-queens-roll.md index 05ccad35354..ac61ce3ec9a 100644 --- a/.changeset/mean-queens-roll.md +++ b/.changeset/mean-queens-roll.md @@ -2,4 +2,4 @@ "@firebase/firestore": patch --- -Fixes an issue that prevents `waitForPendingWrites()` from resolving in background tabs when multi-tab is used. +Fixes an issue that prevents `waitForPendingWrites()` from resolving in background tabs when multi-tab is used (https://github.com/firebase/firebase-js-sdk/issues/3816).