From cddbae31d17b763a87b1870eb1531250b787ecee Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 Feb 2021 14:44:44 -0500 Subject: [PATCH 1/6] Remove enqueued limbo resolutions when a query stops --- .../firestore/src/core/sync_engine_impl.ts | 19 +++++--- .../test/unit/specs/limbo_spec.test.ts | 47 +++++++++++++++++++ .../test/unit/specs/spec_test_runner.ts | 32 +++---------- 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index 7fa3518980f..76e7ae7be90 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -113,6 +113,7 @@ import { ViewChange } from './view'; import { ViewSnapshot } from './view_snapshot'; +import { ResourcePath } from '../model/path'; const LOG_TAG = 'SyncEngine'; @@ -208,9 +209,10 @@ class SyncEngineImpl implements SyncEngine { queriesByTarget = new Map(); /** * The keys of documents that are in limbo for which we haven't yet started a - * limbo resolution query. + * limbo resolution query. The strings in this set are the result of calling + * `key.path.canonicalString()` where `key` is a `DocumentKey` object. */ - enqueuedLimboResolutions: DocumentKey[] = []; + enqueuedLimboResolutions = new Set(); /** * Keeps track of the target ID for each document that is in limbo with an * active target. @@ -876,6 +878,8 @@ function removeLimboTarget( syncEngineImpl: SyncEngineImpl, key: DocumentKey ): void { + syncEngineImpl.enqueuedLimboResolutions.delete(key.path.canonicalString()); + // It's possible that the target already got removed because the query failed. In that case, // the key won't exist in `limboTargetsByKey`. Only do the cleanup if we still have the target. const limboTargetId = syncEngineImpl.activeLimboTargetsByKey.get(key); @@ -927,7 +931,7 @@ function trackLimboChange( const key = limboChange.key; if (!syncEngineImpl.activeLimboTargetsByKey.get(key)) { logDebug(LOG_TAG, 'New document in limbo: ' + key); - syncEngineImpl.enqueuedLimboResolutions.push(key); + syncEngineImpl.enqueuedLimboResolutions.add(key.path.canonicalString()); pumpEnqueuedLimboResolutions(syncEngineImpl); } } @@ -942,11 +946,14 @@ function trackLimboChange( */ function pumpEnqueuedLimboResolutions(syncEngineImpl: SyncEngineImpl): void { while ( - syncEngineImpl.enqueuedLimboResolutions.length > 0 && + syncEngineImpl.enqueuedLimboResolutions.size > 0 && syncEngineImpl.activeLimboTargetsByKey.size < syncEngineImpl.maxConcurrentLimboResolutions ) { - const key = syncEngineImpl.enqueuedLimboResolutions.shift()!; + const keyString = syncEngineImpl.enqueuedLimboResolutions.values().next() + .value; + syncEngineImpl.enqueuedLimboResolutions.delete(keyString); + const key = new DocumentKey(ResourcePath.fromString(keyString)); const limboTargetId = syncEngineImpl.limboTargetIdGenerator.next(); syncEngineImpl.activeLimboResolutionsByTarget.set( limboTargetId, @@ -979,7 +986,7 @@ export function syncEngineGetActiveLimboDocumentResolutions( // Visible for testing export function syncEngineGetEnqueuedLimboDocumentResolutions( syncEngine: SyncEngine -): DocumentKey[] { +): Set { const syncEngineImpl = debugCast(syncEngine, SyncEngineImpl); return syncEngineImpl.enqueuedLimboResolutions; } diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index b8afbf5a647..497f75cd48b 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -922,4 +922,51 @@ describeSpec('Limbo Documents:', [], () => { ); } ); + + specTest( + 'Enqueued limbo resolutions should be removed when query listen stops', + [], + () => { + const doc1 = doc('collection1/doc', 1000, { key: 1 }); + const query1 = query('collection1'); + const filteredQuery1 = query('collection1', filter('key', '==', 1)); + + const doc2 = doc('collection2/doc', 1000, { key: 2 }); + const query2 = query('collection2'); + const filteredQuery2 = query('collection2', filter('key', '==', 2)); + + return ( + spec() + .withGCEnabled(false) + .withMaxConcurrentLimboResolutions(1) + + // Max out the number of active limbo resolutions. + .userListens(filteredQuery1) + .watchAcksFull(filteredQuery1, 1000, doc1) + .expectEvents(filteredQuery1, { added: [doc1] }) + .userUnlistens(filteredQuery1) + .userListens(query1) + .expectEvents(query1, { added: [doc1], fromCache: true }) + .watchAcksFull(query1, 1001) + .expectLimboDocs(doc1.key) + + // Enqueue a limbo resolution from another query. + .userListens(filteredQuery2) + .watchAcksFull(filteredQuery2, 1002, doc2) + .expectEvents(filteredQuery2, { added: [doc2] }) + .userUnlistens(filteredQuery2) + .userListens(query2) + .expectEvents(query2, { added: [doc2], fromCache: true }) + .watchAcksFull(query2, 1003) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs(doc2.key) + + // Stop the other query and ensure that the enqueued limbo resolution + // is removed. + .userUnlistens(query2) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs() + ); + } + ); }); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 57322cfee31..9b4604973ca 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -1008,31 +1008,13 @@ abstract class TestRunner { } private validateEnqueuedLimboDocs(): void { - let actualLimboDocs = new SortedSet(DocumentKey.comparator); - syncEngineGetEnqueuedLimboDocumentResolutions(this.syncEngine).forEach( - key => { - actualLimboDocs = actualLimboDocs.add(key); - } - ); - let expectedLimboDocs = new SortedSet(DocumentKey.comparator); - this.expectedEnqueuedLimboDocs.forEach(key => { - expectedLimboDocs = expectedLimboDocs.add(key); - }); - actualLimboDocs.forEach(key => { - expect(expectedLimboDocs.has(key)).to.equal( - true, - `Found enqueued limbo doc ${key.toString()}, but it was not in ` + - `the set of expected enqueued limbo documents ` + - `(${expectedLimboDocs.toString()})` - ); - }); - expectedLimboDocs.forEach(key => { - expect(actualLimboDocs.has(key)).to.equal( - true, - `Expected doc ${key.toString()} to be enqueued for limbo resolution, ` + - `but it was not in the queue (${actualLimboDocs.toString()})` - ); - }); + const actualLimboDocs = Array.from( + syncEngineGetEnqueuedLimboDocumentResolutions(this.syncEngine) + ).sort(); + const expectedLimboDocs = Array.from(this.expectedEnqueuedLimboDocs, key => + key.path.canonicalString() + ).sort(); + expect(actualLimboDocs).to.deep.equal(expectedLimboDocs); } private async validateActiveTargets(): Promise { From c5f0747a7a9ba92d038f7a60a443e3b1f09fbdca Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 Feb 2021 16:16:58 -0500 Subject: [PATCH 2/6] Add some more spec tests --- .../firestore/src/core/sync_engine_impl.ts | 8 +- .../test/unit/specs/limbo_spec.test.ts | 130 +++++++++++++++--- 2 files changed, 120 insertions(+), 18 deletions(-) diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index 76e7ae7be90..b2d3d646f2b 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -929,9 +929,13 @@ function trackLimboChange( limboChange: AddedLimboDocument ): void { const key = limboChange.key; - if (!syncEngineImpl.activeLimboTargetsByKey.get(key)) { + const keyString = key.path.canonicalString(); + if ( + !syncEngineImpl.activeLimboTargetsByKey.get(key) && + !syncEngineImpl.enqueuedLimboResolutions.has(keyString) + ) { logDebug(LOG_TAG, 'New document in limbo: ' + key); - syncEngineImpl.enqueuedLimboResolutions.add(key.path.canonicalString()); + syncEngineImpl.enqueuedLimboResolutions.add(keyString); pumpEnqueuedLimboResolutions(syncEngineImpl); } } diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 497f75cd48b..c13a54d847f 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -924,7 +924,7 @@ describeSpec('Limbo Documents:', [], () => { ); specTest( - 'Enqueued limbo resolutions should be removed when query listen stops', + 'A limbo resolution for a document should be removed from the queue when the last query listen stops', [], () => { const doc1 = doc('collection1/doc', 1000, { key: 1 }); @@ -934,6 +934,7 @@ describeSpec('Limbo Documents:', [], () => { const doc2 = doc('collection2/doc', 1000, { key: 2 }); const query2 = query('collection2'); const filteredQuery2 = query('collection2', filter('key', '==', 2)); + const filteredQuery3 = query('collection2', filter('key', '>=', 2)); return ( spec() @@ -941,31 +942,128 @@ describeSpec('Limbo Documents:', [], () => { .withMaxConcurrentLimboResolutions(1) // Max out the number of active limbo resolutions. - .userListens(filteredQuery1) - .watchAcksFull(filteredQuery1, 1000, doc1) - .expectEvents(filteredQuery1, { added: [doc1] }) - .userUnlistens(filteredQuery1) .userListens(query1) - .expectEvents(query1, { added: [doc1], fromCache: true }) - .watchAcksFull(query1, 1001) + .watchAcksFull(query1, 1000, doc1) + .expectEvents(query1, { added: [doc1] }) + .userUnlistens(query1) + .userListens(filteredQuery1) + .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) + .watchAcksFull(filteredQuery1, 1001) .expectLimboDocs(doc1.key) - // Enqueue a limbo resolution from another query. + // Enqueue a limbo resolution for doc2. + .userListens(query2) + .watchAcksFull(query2, 1002, doc2) + .expectEvents(query2, { added: [doc2] }) + .userUnlistens(query2) .userListens(filteredQuery2) - .watchAcksFull(filteredQuery2, 1002, doc2) - .expectEvents(filteredQuery2, { added: [doc2] }) + .expectEvents(filteredQuery2, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery2, 1003) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs(doc2.key) + + // Start another query that puts doc2 into limbo again. + .userListens(filteredQuery3) + .expectEvents(filteredQuery3, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery3, 1004) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs(doc2.key) + + // Stop one of the queries that enqueued a limbo resolution for doc2; + // verify that doc2 is not removed from the limbo resolution queue. + .userUnlistens(filteredQuery3) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs(doc2.key) + + // Stop the other query that enqueued a limbo resolution for doc2; + // verify that doc2 *is* removed from the limbo resolution queue. .userUnlistens(filteredQuery2) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs() + ); + } + ); + + specTest( + 'A limbo resolution for a document should not be started if one is already active', + [], + () => { + const doc1 = doc('collection/doc', 1000, { key: 1 }); + const query1 = query('collection'); + const filteredQuery1 = query('collection', filter('key', '==', 1)); + const filteredQuery2 = query('collection', filter('key', '>=', 1)); + + return ( + spec() + .withGCEnabled(false) + + // Start a limbo resolution listen for a document. + .userListens(query1) + .watchAcksFull(query1, 1000, doc1) + .expectEvents(query1, { added: [doc1] }) + .userUnlistens(query1) + .userListens(filteredQuery1) + .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) + .watchAcksFull(filteredQuery1, 1001) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs() + + // Put the same document into limbo in a different query. + .userListens(filteredQuery2) + .expectEvents(filteredQuery2, { added: [doc1], fromCache: true }) + .watchAcksFull(filteredQuery2, 1002) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs() + ); + } + ); + + specTest( + 'A limbo resolution for a document should not be enqueued if one is already enqueued', + [], + () => { + const doc1 = doc('collection1/doc1', 1000, { key: 1 }); + const query1 = query('collection1'); + const filteredQuery1 = query('collection1', filter('key', '==', 1)); + const doc2 = doc('collection2/doc2', 1000, { key: 2 }); + const query2 = query('collection2'); + const filteredQuery2 = query('collection2', filter('key', '==', 2)); + const filteredQuery3 = query('collection2', filter('key', '>=', 2)); + + return ( + spec() + .withGCEnabled(false) + .withMaxConcurrentLimboResolutions(1) + + // Max out the number of active limbo resolutions. + .userListens(query1) + .watchAcksFull(query1, 1000, doc1) + .expectEvents(query1, { added: [doc1] }) + .userUnlistens(query1) + .userListens(filteredQuery1) + .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) + .watchAcksFull(filteredQuery1, 1001) + .expectLimboDocs(doc1.key) + + // Start a limbo resolution listen for a different document. .userListens(query2) - .expectEvents(query2, { added: [doc2], fromCache: true }) - .watchAcksFull(query2, 1003) + .watchAcksFull(query2, 1002, doc2) + .expectEvents(query2, { added: [doc2] }) + .userUnlistens(query2) + .userListens(filteredQuery2) + .expectEvents(filteredQuery2, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery2, 1003) .expectLimboDocs(doc1.key) .expectEnqueuedLimboDocs(doc2.key) - // Stop the other query and ensure that the enqueued limbo resolution - // is removed. - .userUnlistens(query2) + // Put the the "different" document into limbo in a different query + // and verify that it's not added to the limbo resolution queue a + // second time. + .userListens(filteredQuery3) + .expectEvents(filteredQuery3, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery3, 1004) .expectLimboDocs(doc1.key) - .expectEnqueuedLimboDocs() + .expectEnqueuedLimboDocs(doc2.key) ); } ); From b0ea581abb9fcb18bcaa2d2169e59c78dd639b62 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 Feb 2021 16:22:32 -0500 Subject: [PATCH 3/6] Clean up some inline comments --- .../firestore/test/unit/specs/limbo_spec.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index c13a54d847f..71da7a251a9 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -997,7 +997,7 @@ describeSpec('Limbo Documents:', [], () => { spec() .withGCEnabled(false) - // Start a limbo resolution listen for a document. + // Start a limbo resolution listen for a document (doc1). .userListens(query1) .watchAcksFull(query1, 1000, doc1) .expectEvents(query1, { added: [doc1] }) @@ -1008,7 +1008,8 @@ describeSpec('Limbo Documents:', [], () => { .expectLimboDocs(doc1.key) .expectEnqueuedLimboDocs() - // Put the same document into limbo in a different query. + // Put doc1 into limbo in a different query; verify that another limbo + // resolution is neither started nor enqueued. .userListens(filteredQuery2) .expectEvents(filteredQuery2, { added: [doc1], fromCache: true }) .watchAcksFull(filteredQuery2, 1002) @@ -1045,7 +1046,7 @@ describeSpec('Limbo Documents:', [], () => { .watchAcksFull(filteredQuery1, 1001) .expectLimboDocs(doc1.key) - // Start a limbo resolution listen for a different document. + // Start a limbo resolution listen for a different document (doc2). .userListens(query2) .watchAcksFull(query2, 1002, doc2) .expectEvents(query2, { added: [doc2] }) @@ -1056,9 +1057,8 @@ describeSpec('Limbo Documents:', [], () => { .expectLimboDocs(doc1.key) .expectEnqueuedLimboDocs(doc2.key) - // Put the the "different" document into limbo in a different query - // and verify that it's not added to the limbo resolution queue a - // second time. + // Put doc2 into limbo in a different query and verify that it's not + // added to the limbo resolution queue again. .userListens(filteredQuery3) .expectEvents(filteredQuery3, { added: [doc2], fromCache: true }) .watchAcksFull(filteredQuery3, 1004) From 35e6d3e4e471eef8d8131c91de7261b0fa45ab47 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 Feb 2021 16:35:13 -0500 Subject: [PATCH 4/6] add changeset --- .changeset/eight-bananas-nail.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eight-bananas-nail.md diff --git a/.changeset/eight-bananas-nail.md b/.changeset/eight-bananas-nail.md new file mode 100644 index 00000000000..010a5902088 --- /dev/null +++ b/.changeset/eight-bananas-nail.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Fix a bug where local cache inconsistencies were unnecessarily being resolved. From 9bbd652440061c1dd6a4a2332915972d7b3f9d01 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 Feb 2021 19:59:54 -0500 Subject: [PATCH 5/6] Fix lint errors --- packages/firestore/src/core/sync_engine_impl.ts | 2 +- packages/firestore/test/unit/specs/spec_test_runner.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index b2d3d646f2b..58da48a1084 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -51,6 +51,7 @@ import { MaybeDocument, NoDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { Mutation } from '../model/mutation'; import { MutationBatchResult } from '../model/mutation_batch'; +import { ResourcePath } from '../model/path'; import { RemoteEvent, TargetChange } from '../remote/remote_event'; import { canUseNetwork, @@ -113,7 +114,6 @@ import { ViewChange } from './view'; import { ViewSnapshot } from './view_snapshot'; -import { ResourcePath } from '../model/path'; const LOG_TAG = 'SyncEngine'; diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 9b4604973ca..9657c0f4dd0 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -118,7 +118,6 @@ import { primitiveComparator } from '../../../src/util/misc'; import { forEach, objectSize } from '../../../src/util/obj'; import { ObjectMap } from '../../../src/util/obj_map'; import { Deferred, sequence } from '../../../src/util/promise'; -import { SortedSet } from '../../../src/util/sorted_set'; import { byteStringFromString, deletedDoc, From 03a9c2241d6f1105c80e30c1905cedbdee49281a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 4 Feb 2021 13:22:26 -0500 Subject: [PATCH 6/6] Apply code review feedback --- .changeset/eight-bananas-nail.md | 2 +- .../firestore/src/core/sync_engine_impl.ts | 4 + .../test/unit/specs/limbo_spec.test.ts | 164 +++++++++--------- .../test/unit/specs/spec_test_runner.ts | 9 +- 4 files changed, 93 insertions(+), 86 deletions(-) diff --git a/.changeset/eight-bananas-nail.md b/.changeset/eight-bananas-nail.md index 010a5902088..e92495c41de 100644 --- a/.changeset/eight-bananas-nail.md +++ b/.changeset/eight-bananas-nail.md @@ -2,4 +2,4 @@ '@firebase/firestore': patch --- -Fix a bug where local cache inconsistencies were unnecessarily being resolved. +Fixes a bug where local cache inconsistencies were unnecessarily being resolved. diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index 58da48a1084..2424fa47331 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -211,6 +211,10 @@ class SyncEngineImpl implements SyncEngine { * The keys of documents that are in limbo for which we haven't yet started a * limbo resolution query. The strings in this set are the result of calling * `key.path.canonicalString()` where `key` is a `DocumentKey` object. + * + * The `Set` type was chosen because it provides efficient lookup and removal + * of arbitrary elements and it also maintains insertion order, providing the + * desired queue-like FIFO semantics. */ enqueuedLimboResolutions = new Set(); /** diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 71da7a251a9..0188f04b309 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -924,60 +924,34 @@ describeSpec('Limbo Documents:', [], () => { ); specTest( - 'A limbo resolution for a document should be removed from the queue when the last query listen stops', + 'A limbo resolution for a document should not be started if one is already active', [], () => { - const doc1 = doc('collection1/doc', 1000, { key: 1 }); - const query1 = query('collection1'); - const filteredQuery1 = query('collection1', filter('key', '==', 1)); - - const doc2 = doc('collection2/doc', 1000, { key: 2 }); - const query2 = query('collection2'); - const filteredQuery2 = query('collection2', filter('key', '==', 2)); - const filteredQuery3 = query('collection2', filter('key', '>=', 2)); + const doc1 = doc('collection/doc', 1000, { key: 1 }); + const fullQuery = query('collection'); + const filteredQuery1 = query('collection', filter('key', '==', 1)); + const filteredQuery2 = query('collection', filter('key', '>=', 1)); return ( spec() .withGCEnabled(false) - .withMaxConcurrentLimboResolutions(1) - // Max out the number of active limbo resolutions. - .userListens(query1) - .watchAcksFull(query1, 1000, doc1) - .expectEvents(query1, { added: [doc1] }) - .userUnlistens(query1) + // Start a limbo resolution listen for a document (doc1). + .userListens(fullQuery) + .watchAcksFull(fullQuery, 1000, doc1) + .expectEvents(fullQuery, { added: [doc1] }) + .userUnlistens(fullQuery) .userListens(filteredQuery1) .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) .watchAcksFull(filteredQuery1, 1001) .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs() - // Enqueue a limbo resolution for doc2. - .userListens(query2) - .watchAcksFull(query2, 1002, doc2) - .expectEvents(query2, { added: [doc2] }) - .userUnlistens(query2) + // Put doc1 into limbo in a different query; verify that another limbo + // resolution is neither started nor enqueued. .userListens(filteredQuery2) - .expectEvents(filteredQuery2, { added: [doc2], fromCache: true }) - .watchAcksFull(filteredQuery2, 1003) - .expectLimboDocs(doc1.key) - .expectEnqueuedLimboDocs(doc2.key) - - // Start another query that puts doc2 into limbo again. - .userListens(filteredQuery3) - .expectEvents(filteredQuery3, { added: [doc2], fromCache: true }) - .watchAcksFull(filteredQuery3, 1004) - .expectLimboDocs(doc1.key) - .expectEnqueuedLimboDocs(doc2.key) - - // Stop one of the queries that enqueued a limbo resolution for doc2; - // verify that doc2 is not removed from the limbo resolution queue. - .userUnlistens(filteredQuery3) - .expectLimboDocs(doc1.key) - .expectEnqueuedLimboDocs(doc2.key) - - // Stop the other query that enqueued a limbo resolution for doc2; - // verify that doc2 *is* removed from the limbo resolution queue. - .userUnlistens(filteredQuery2) + .expectEvents(filteredQuery2, { added: [doc1], fromCache: true }) + .watchAcksFull(filteredQuery2, 1002) .expectLimboDocs(doc1.key) .expectEnqueuedLimboDocs() ); @@ -985,51 +959,66 @@ describeSpec('Limbo Documents:', [], () => { ); specTest( - 'A limbo resolution for a document should not be started if one is already active', + 'A limbo resolution for a document should not be enqueued if one is already enqueued', [], () => { - const doc1 = doc('collection/doc', 1000, { key: 1 }); - const query1 = query('collection'); - const filteredQuery1 = query('collection', filter('key', '==', 1)); - const filteredQuery2 = query('collection', filter('key', '>=', 1)); + const doc1 = doc('collection1/doc1', 1000, { key: 1 }); + const fullQuery1 = query('collection1'); + const filteredQuery1 = query('collection1', filter('key', '==', 1)); + const doc2 = doc('collection2/doc2', 1000, { key: 2 }); + const fullQuery2 = query('collection2'); + const filteredQuery2a = query('collection2', filter('key', '==', 2)); + const filteredQuery2b = query('collection2', filter('key', '>=', 2)); return ( spec() .withGCEnabled(false) + .withMaxConcurrentLimboResolutions(1) - // Start a limbo resolution listen for a document (doc1). - .userListens(query1) - .watchAcksFull(query1, 1000, doc1) - .expectEvents(query1, { added: [doc1] }) - .userUnlistens(query1) + // Max out the number of active limbo resolutions. + .userListens(fullQuery1) + .watchAcksFull(fullQuery1, 1000, doc1) + .expectEvents(fullQuery1, { added: [doc1] }) + .userUnlistens(fullQuery1) .userListens(filteredQuery1) .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) .watchAcksFull(filteredQuery1, 1001) .expectLimboDocs(doc1.key) - .expectEnqueuedLimboDocs() - // Put doc1 into limbo in a different query; verify that another limbo - // resolution is neither started nor enqueued. - .userListens(filteredQuery2) - .expectEvents(filteredQuery2, { added: [doc1], fromCache: true }) - .watchAcksFull(filteredQuery2, 1002) + // Start a limbo resolution listen for a different document (doc2). + .userListens(fullQuery2) + .watchAcksFull(fullQuery2, 1002, doc2) + .expectEvents(fullQuery2, { added: [doc2] }) + .userUnlistens(fullQuery2) + .userListens(filteredQuery2a) + .expectEvents(filteredQuery2a, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery2a, 1003) .expectLimboDocs(doc1.key) - .expectEnqueuedLimboDocs() + .expectEnqueuedLimboDocs(doc2.key) + + // Put doc2 into limbo in a different query and verify that it's not + // added to the limbo resolution queue again. + .userListens(filteredQuery2b) + .expectEvents(filteredQuery2b, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery2b, 1004) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs(doc2.key) ); } ); specTest( - 'A limbo resolution for a document should not be enqueued if one is already enqueued', + 'A limbo resolution for a document should be removed from the queue when the last query listen stops', [], () => { - const doc1 = doc('collection1/doc1', 1000, { key: 1 }); - const query1 = query('collection1'); + const doc1 = doc('collection1/doc', 1000, { key: 1 }); + const fullQuery1 = query('collection1'); const filteredQuery1 = query('collection1', filter('key', '==', 1)); - const doc2 = doc('collection2/doc2', 1000, { key: 2 }); - const query2 = query('collection2'); - const filteredQuery2 = query('collection2', filter('key', '==', 2)); - const filteredQuery3 = query('collection2', filter('key', '>=', 2)); + + const doc2 = doc('collection2/doc', 1000, { key: 2 }); + const fullQuery2 = query('collection2'); + const filteredQuery2a = query('collection2', filter('key', '==', 2)); + const filteredQuery2b = query('collection2', filter('key', '>=', 2)); return ( spec() @@ -1037,33 +1026,44 @@ describeSpec('Limbo Documents:', [], () => { .withMaxConcurrentLimboResolutions(1) // Max out the number of active limbo resolutions. - .userListens(query1) - .watchAcksFull(query1, 1000, doc1) - .expectEvents(query1, { added: [doc1] }) - .userUnlistens(query1) + .userListens(fullQuery1) + .watchAcksFull(fullQuery1, 1000, doc1) + .expectEvents(fullQuery1, { added: [doc1] }) + .userUnlistens(fullQuery1) .userListens(filteredQuery1) .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) .watchAcksFull(filteredQuery1, 1001) .expectLimboDocs(doc1.key) - // Start a limbo resolution listen for a different document (doc2). - .userListens(query2) - .watchAcksFull(query2, 1002, doc2) - .expectEvents(query2, { added: [doc2] }) - .userUnlistens(query2) - .userListens(filteredQuery2) - .expectEvents(filteredQuery2, { added: [doc2], fromCache: true }) - .watchAcksFull(filteredQuery2, 1003) + // Enqueue a limbo resolution for doc2. + .userListens(fullQuery2) + .watchAcksFull(fullQuery2, 1002, doc2) + .expectEvents(fullQuery2, { added: [doc2] }) + .userUnlistens(fullQuery2) + .userListens(filteredQuery2a) + .expectEvents(filteredQuery2a, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery2a, 1003) .expectLimboDocs(doc1.key) .expectEnqueuedLimboDocs(doc2.key) - // Put doc2 into limbo in a different query and verify that it's not - // added to the limbo resolution queue again. - .userListens(filteredQuery3) - .expectEvents(filteredQuery3, { added: [doc2], fromCache: true }) - .watchAcksFull(filteredQuery3, 1004) + // Start another query that puts doc2 into limbo again. + .userListens(filteredQuery2b) + .expectEvents(filteredQuery2b, { added: [doc2], fromCache: true }) + .watchAcksFull(filteredQuery2b, 1004) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs(doc2.key) + + // Stop one of the queries that enqueued a limbo resolution for doc2; + // verify that doc2 is not removed from the limbo resolution queue. + .userUnlistens(filteredQuery2b) .expectLimboDocs(doc1.key) .expectEnqueuedLimboDocs(doc2.key) + + // Stop the other query that enqueued a limbo resolution for doc2; + // verify that doc2 *is* removed from the limbo resolution queue. + .userUnlistens(filteredQuery2a) + .expectLimboDocs(doc1.key) + .expectEnqueuedLimboDocs() ); } ); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 9657c0f4dd0..abd05973c5d 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -1009,11 +1009,14 @@ abstract class TestRunner { private validateEnqueuedLimboDocs(): void { const actualLimboDocs = Array.from( syncEngineGetEnqueuedLimboDocumentResolutions(this.syncEngine) - ).sort(); + ); const expectedLimboDocs = Array.from(this.expectedEnqueuedLimboDocs, key => key.path.canonicalString() - ).sort(); - expect(actualLimboDocs).to.deep.equal(expectedLimboDocs); + ); + expect(actualLimboDocs).to.have.members( + expectedLimboDocs, + 'The set of enqueued limbo documents is incorrect' + ); } private async validateActiveTargets(): Promise {