From e07baa64913f907d5beeeb0a92ed0c1d63ad1355 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 26 May 2020 23:10:07 -0700 Subject: [PATCH 1/2] Address potential IndexedDB failure in synchronizeQueryViewsAndRaiseSnapshots --- packages/firestore/src/core/sync_engine.ts | 10 +-- .../src/local/indexeddb_persistence.ts | 6 +- .../test/unit/specs/recovery_spec.test.ts | 80 ++++++++++++++++++- 3 files changed, 85 insertions(+), 11 deletions(-) diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index 0204f403e50..1c8a0aee75f 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -1110,14 +1110,10 @@ export class MultiTabSyncEngine extends SyncEngine const queries = this.queriesByTarget.get(targetId); if (queries && queries.length !== 0) { - // For queries that have a local View, we need to update their state - // in LocalStore (as the resume token and the snapshot version + // For queries that have a local View, we fetch their current state + // from LocalStore (as the resume token and the snapshot version // might have changed) and reconcile their views with the persisted // state (the list of syncedDocuments may have gotten out of sync). - await this.localStore.releaseTarget( - targetId, - /*keepPersistedTargetData=*/ true - ); targetData = await this.localStore.allocateTarget( queries[0].toTarget() ); @@ -1136,7 +1132,7 @@ export class MultiTabSyncEngine extends SyncEngine } else { debugAssert( transitionToPrimary, - 'A secondary tab should never have an active target without an active query.' + 'A secondary tab should never have an active view without an active target.' ); // For queries that never executed on this client, we need to // allocate the target in LocalStore and initialize a new View. diff --git a/packages/firestore/src/local/indexeddb_persistence.ts b/packages/firestore/src/local/indexeddb_persistence.ts index 7733706c3ac..a0207e7e1c2 100644 --- a/packages/firestore/src/local/indexeddb_persistence.ts +++ b/packages/firestore/src/local/indexeddb_persistence.ts @@ -404,7 +404,7 @@ export class IndexedDbPersistence implements Persistence { return this.verifyPrimaryLease(txn).next(success => { if (!success) { this.isPrimary = false; - this.queue.enqueueAndForget(() => + this.queue.enqueueRetryable(() => this.primaryStateListener(false) ); } @@ -444,7 +444,7 @@ export class IndexedDbPersistence implements Persistence { }) .then(isPrimary => { if (this.isPrimary !== isPrimary) { - this.queue.enqueueAndForget(() => + this.queue.enqueueRetryable(() => this.primaryStateListener(isPrimary) ); } @@ -805,7 +805,7 @@ export class IndexedDbPersistence implements Persistence { `Failed to obtain primary lease for action '${action}'.` ); this.isPrimary = false; - this.queue.enqueueAndForget(() => + this.queue.enqueueRetryable(() => this.primaryStateListener(false) ); throw new FirestoreError( diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts index a302036c593..f16d9d87ccc 100644 --- a/packages/firestore/test/unit/specs/recovery_spec.test.ts +++ b/packages/firestore/test/unit/specs/recovery_spec.test.ts @@ -88,7 +88,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { ); specTest( - 'Query raises events in secondary client (with recovery)', + 'Query raises events in secondary client (with recovery)', ['multi-client'], () => { const query = Query.atPath(path('collection')); @@ -144,6 +144,84 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { } ); + specTest( + 'Query with active view recovers after primary tab failover (with recovery)', + ['multi-client'], + () => { + const query = Query.atPath(path('collection')); + const docA = doc('collection/a', 1000, { key: 'a' }); + const docB = doc('collection/b', 2000, { key: 'b' }); + + return ( + client(0, false) + .expectPrimaryState(true) + .client(1) + // Register a query in the secondary client + .userListens(query) + .client(0) + .expectListen(query) + .watchAcksFull(query, 1000, docA) + // Shutdown the primary client to release its lease + .shutdown() + .client(1) + .expectEvents(query, { added: [docA] }) + // Run the lease refresh to attempt taking over the primary lease. The + // first lease refresh fails with a simulated transaction failure. + .failDatabaseTransactions('Allocate target') + .runTimer(TimerId.ClientMetadataRefresh) + .expectPrimaryState(false) + .recoverDatabase() + .runTimer(TimerId.AsyncQueueRetry) + .expectPrimaryState(true) + .expectListen(query, 'resume-token-1000') + .watchAcksFull(query, 2000, docB) + .expectEvents(query, { added: [docB] }) + ); + } + ); + + specTest( + 'Query without active view recovers after primary tab failover (with recovery)', + ['multi-client'], + () => { + const query = Query.atPath(path('collection')); + const docA = doc('collection/a', 1000, { key: 'a' }); + const docB = doc('collection/b', 2000, { key: 'b' }); + + return ( + client(0, false) + .expectPrimaryState(true) + // Initialize a second client that doesn't have any active targets + .client(1) + .client(2) + // Register a query in the third client + .userListens(query) + .client(0) + .expectListen(query) + .watchAcksFull(query, 1000, docA) + .client(2) + .expectEvents(query, { added: [docA] }) + .client(0) + // Shutdown the primary client to release its lease + .shutdown() + .client(1) + // Run the lease refresh in the second client, which does not yet have + // an active view for the third client's query. The lease refresh fails + // at first, but then recovers and initializes the view. + .failDatabaseTransactions('Allocate target') + .runTimer(TimerId.ClientMetadataRefresh) + .expectPrimaryState(false) + .recoverDatabase() + .runTimer(TimerId.AsyncQueueRetry) + .expectPrimaryState(true) + .expectListen(query, 'resume-token-1000') + .watchAcksFull(query, 2000, docB) + .client(2) + .expectEvents(query, { added: [docB] }) + ); + } + ); + specTest( 'Ignores intermittent lease refresh failures (with recovery)', ['multi-client'], From 7997585e9e74c06dc9ddaf3378190d6eebc6e4fc Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 26 May 2020 23:46:10 -0700 Subject: [PATCH 2/2] Use in-memory data when newer --- packages/firestore/src/local/local_store.ts | 12 +++++++++++- .../firestore/test/unit/specs/listen_spec.test.ts | 2 +- .../firestore/test/unit/specs/recovery_spec.test.ts | 4 ++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index 361cbd48b7f..d09f48089b2 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -831,7 +831,17 @@ export class LocalStore { }); }) .then(targetData => { - if (this.targetDataByTarget.get(targetData.targetId) === null) { + // If Multi-Tab is enabled, the existing target data may be newer than + // the in-memory data + const cachedTargetData = this.targetDataByTarget.get( + targetData.targetId + ); + if ( + cachedTargetData === null || + targetData.snapshotVersion.compareTo( + cachedTargetData.snapshotVersion + ) > 0 + ) { this.targetDataByTarget = this.targetDataByTarget.insert( targetData.targetId, targetData diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 5845e4e6f54..a34d14298a6 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -17,7 +17,7 @@ import { Query } from '../../../src/core/query'; import { Code } from '../../../src/util/error'; -import { deletedDoc, doc, filter, path, orderBy } from '../../util/helpers'; +import { deletedDoc, doc, filter, orderBy, path } from '../../util/helpers'; import { TimerId } from '../../../src/util/async_queue'; import { describeSpec, specTest } from './describe_spec'; diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts index f16d9d87ccc..26a8208bbf3 100644 --- a/packages/firestore/test/unit/specs/recovery_spec.test.ts +++ b/packages/firestore/test/unit/specs/recovery_spec.test.ts @@ -153,7 +153,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { const docB = doc('collection/b', 2000, { key: 'b' }); return ( - client(0, false) + client(0) .expectPrimaryState(true) .client(1) // Register a query in the secondary client @@ -189,7 +189,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { const docB = doc('collection/b', 2000, { key: 'b' }); return ( - client(0, false) + client(0) .expectPrimaryState(true) // Initialize a second client that doesn't have any active targets .client(1)