From e6b076824f932f85107e432bdf08ce49e826934f Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 1 Oct 2018 17:29:24 -0700 Subject: [PATCH 1/5] Detect and recover from GCed Remote Document Changelog --- packages/firestore/src/core/sync_engine.ts | 36 +++++++-- .../local/indexeddb_remote_document_cache.ts | 75 ++++++++++++++----- packages/firestore/src/local/local_store.ts | 10 +++ .../src/local/memory_remote_document_cache.ts | 7 ++ .../src/local/remote_document_cache.ts | 11 +++ .../unit/local/remote_document_cache.test.ts | 61 +++++++++++++-- .../unit/local/test_remote_document_cache.ts | 34 ++++++++- packages/firestore/tslint.json | 2 +- 8 files changed, 200 insertions(+), 36 deletions(-) diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index 30e77853f4a..2395375f332 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -42,6 +42,7 @@ import { SortedMap } from '../util/sorted_map'; import { isNullOrUndefined } from '../util/types'; import { isPrimaryLeaseLostError } from '../local/indexeddb_persistence'; +import { isRemoteDocumentChangesGarbageCollectedError } from '../local/indexeddb_remote_document_cache'; import { ClientId, SharedClientState } from '../local/shared_client_state'; import { QueryTargetState, @@ -1001,14 +1002,33 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer { switch (state) { case 'current': case 'not-current': { - const changes = await this.localStore.getNewDocumentChanges(); - const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange( - targetId, - state === 'current' - ); - return this.emitNewSnapsAndNotifyLocalStore( - changes, - synthesizedRemoteEvent + return this.localStore.getNewDocumentChanges().then( + async changes => { + // tslint and prettier disagree about their preferred line length. + // tslint:disable-next-line:max-line-length + const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange( + targetId, + state === 'current' + ); + await this.emitNewSnapsAndNotifyLocalStore( + changes, + synthesizedRemoteEvent + ); + }, + async err => { + if (isRemoteDocumentChangesGarbageCollectedError(err)) { + const activeTargets: TargetId[] = []; + objUtils.forEachNumber(this.queryViewsByTarget, target => + activeTargets.push(target) + ); + await this.synchronizeQueryViewsAndRaiseSnapshots( + activeTargets + ); + await this.localStore.resetLastDocumentChangeId(); + } else { + throw err; + } + } ); } case 'rejected': { diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index afd83ec859e..5dc9e052f08 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -27,6 +27,7 @@ import { DocumentKey } from '../model/document_key'; import { SnapshotVersion } from '../core/snapshot_version'; import { assert } from '../util/assert'; +import { Code, FirestoreError } from '../util/error'; import { IndexedDbPersistence } from './indexeddb_persistence'; import { DbRemoteDocument, @@ -40,6 +41,10 @@ import { PersistencePromise } from './persistence_promise'; import { RemoteDocumentCache } from './remote_document_cache'; import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db'; +const REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG = + 'The remote document changelog no longer contains all changes for all ' + + 'local query views. It may be necessary to rebuild these views.'; + export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { /** The last id read by `getNewDocumentChanges()`. */ private _lastProcessedDocumentChangeId = 0; @@ -69,21 +74,11 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { */ // PORTING NOTE: This is only used for multi-tab synchronization. start(transaction: SimpleDbTransaction): PersistencePromise { - // If there are no existing changes, we set `lastProcessedDocumentChangeId` - // to 0 since IndexedDb's auto-generated keys start at 1. - this._lastProcessedDocumentChangeId = 0; - const store = SimpleDb.getStore< DbRemoteDocumentChangesKey, DbRemoteDocumentChanges >(transaction, DbRemoteDocumentChanges.store); - return store.iterate( - { keysOnly: true, reverse: true }, - (key, value, control) => { - this._lastProcessedDocumentChangeId = key; - control.done(); - } - ); + return this.synchronizeLastDocumentChangeId(store); } addEntries( @@ -172,18 +167,26 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { let changedDocs = maybeDocumentMap(); const range = IDBKeyRange.lowerBound( - this._lastProcessedDocumentChangeId, - /*lowerOpen=*/ true + this._lastProcessedDocumentChangeId + 1 ); + let firstIteration = true; - // TODO(b/114228464): Another client may have garbage collected the remote - // document changelog if our client was throttled for more than 30 minutes. - // We can detect this if the `lastProcessedDocumentChangeId` entry is no - // longer in the changelog. It is possible to recover from this state, - // either by replaying the entire remote document cache or by re-executing - // all queries against the local store. return documentChangesStore(transaction) .iterate({ range }, (_, documentChange) => { + if (firstIteration) { + // If our client was throttled for more than 30 minutes, another + // client may have garbage collected the remote document changelog. + if (this._lastProcessedDocumentChangeId + 1 !== documentChange.id) { + return PersistencePromise.reject( + new FirestoreError( + Code.DATA_LOSS, + REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG + ) + ); + } + firstIteration = false; + } + changedKeys = changedKeys.unionWith( this.serializer.fromDbResourcePaths(documentChange.changes) ); @@ -217,6 +220,40 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { const range = IDBKeyRange.upperBound(changeId); return documentChangesStore(transaction).delete(range); } + + resetLastProcessedDocumentChange( + transaction: PersistenceTransaction + ): PersistencePromise { + const store = documentChangesStore(transaction); + return this.synchronizeLastDocumentChangeId(store); + } + + private synchronizeLastDocumentChangeId( + documentChangesStore: SimpleDbStore< + DbRemoteDocumentChangesKey, + DbRemoteDocumentChanges + > + ): PersistencePromise { + // If there are no existing changes, we set `lastProcessedDocumentChangeId` + // to 0 since IndexedDb's auto-generated keys start at 1. + this._lastProcessedDocumentChangeId = 0; + return documentChangesStore.iterate( + { keysOnly: true, reverse: true }, + (key, value, control) => { + this._lastProcessedDocumentChangeId = key; + control.done(); + } + ); + } +} + +export function isRemoteDocumentChangesGarbageCollectedError( + err: FirestoreError +): boolean { + return ( + err.code === Code.DATA_LOSS && + err.message === REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG + ); } /** diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index 3ca9cecbee6..d6a3a81205d 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -869,4 +869,14 @@ export class LocalStore { } ); } + + resetLastDocumentChangeId(): Promise { + return this.persistence.runTransaction( + 'Reset last document change ID', + 'readonly', + txn => { + return this.remoteDocuments.resetLastProcessedDocumentChange(txn); + } + ); + } } diff --git a/packages/firestore/src/local/memory_remote_document_cache.ts b/packages/firestore/src/local/memory_remote_document_cache.ts index ad945b68b05..bee25036aa0 100644 --- a/packages/firestore/src/local/memory_remote_document_cache.ts +++ b/packages/firestore/src/local/memory_remote_document_cache.ts @@ -99,4 +99,11 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache { return PersistencePromise.resolve(changedDocs); } + + resetLastProcessedDocumentChange( + transaction: PersistenceTransaction + ): PersistencePromise { + this.newDocumentChanges = documentKeySet(); + return PersistencePromise.resolve(); + } } diff --git a/packages/firestore/src/local/remote_document_cache.ts b/packages/firestore/src/local/remote_document_cache.ts index ae030a0169b..f22b4cb0b7d 100644 --- a/packages/firestore/src/local/remote_document_cache.ts +++ b/packages/firestore/src/local/remote_document_cache.ts @@ -94,4 +94,15 @@ export interface RemoteDocumentCache { getNewDocumentChanges( transaction: PersistenceTransaction ): PersistencePromise; + + /** + * Skips all existing change log entries in IndexedDb and moves the change log + * cursor past the last existing change. This method should only be called if + * `getNewDocumentChanges()`can no longer replay all changes and all views + * have already been manually synchronized. + */ + // PORTING NOTE: This is only used for multi-tab synchronization. + resetLastProcessedDocumentChange( + transaction: PersistenceTransaction + ): PersistencePromise; } diff --git a/packages/firestore/test/unit/local/remote_document_cache.test.ts b/packages/firestore/test/unit/local/remote_document_cache.test.ts index b9606b61fc8..f122058ff5e 100644 --- a/packages/firestore/test/unit/local/remote_document_cache.test.ts +++ b/packages/firestore/test/unit/local/remote_document_cache.test.ts @@ -28,13 +28,18 @@ import { removedDoc } from '../../util/helpers'; -import { IndexedDbRemoteDocumentCache } from '../../../src/local/indexeddb_remote_document_cache'; +import { + IndexedDbRemoteDocumentCache, + isRemoteDocumentChangesGarbageCollectedError +} from '../../../src/local/indexeddb_remote_document_cache'; import { DbRemoteDocumentChanges, DbRemoteDocumentChangesKey } from '../../../src/local/indexeddb_schema'; import { MaybeDocumentMap } from '../../../src/model/collections'; +import { fail } from '../../../src/util/assert'; import * as persistenceHelpers from './persistence_test_helpers'; +import { INDEXEDDB_TEST_SERIALIZER } from './persistence_test_helpers'; import { TestRemoteDocumentCache } from './test_remote_document_cache'; // Helpers for use throughout tests. @@ -115,10 +120,54 @@ describe('IndexedDbRemoteDocumentCache', () => { persistence, persistence.getRemoteDocumentCache() ); - const changedDocs = await cache.getNextDocumentChanges(); + const changedDocs = await cache.getNewDocumentChanges(); assertMatches([], changedDocs); }); + it('can recover from garbage collected change log', async () => { + // This test is meant to simulate the recovery from a garbage collected + // document change log. + // The tests adds four changes (via the `writer`). After the first change is + // processed by the reader, the writer garbage collects the first and second + // change. When reader then reads the new changes, it notices that a change + // is missing. The test then uses `resetLastProcessedDocumentChange` to + // simulate a successful recovery. + + const writerCache = new TestRemoteDocumentCache( + persistence, + persistence.getRemoteDocumentCache() + ); + const readerCache = new TestRemoteDocumentCache( + persistence, + new IndexedDbRemoteDocumentCache(INDEXEDDB_TEST_SERIALIZER, true) + ); + + await writerCache.addEntries([doc('a/1', 1, DOC_DATA)]); + let changedDocs = await readerCache.getNewDocumentChanges(); + assertMatches([doc('a/1', 1, DOC_DATA)], changedDocs); + + await writerCache.addEntries([doc('a/2', 2, DOC_DATA)]); + await writerCache.addEntries([doc('a/3', 3, DOC_DATA)]); + // Garbage collect change 1 and 2, but not change 3. + await writerCache.removeDocumentChangesThroughChangeId(2); + + await readerCache + .getNewDocumentChanges() + .then( + () => fail('Missing expected error'), + err => + expect(isRemoteDocumentChangesGarbageCollectedError(err)).to.be.ok + ); + + // Recover by moving moving the change log cursor to the last existing + // entry. We assume that as part of the recovery, all relevant changes have + // been processed manually (see `SyncEngine.applyTargetState`). + await readerCache.resetLastProcessDocumentChange(); + await writerCache.addEntries([doc('a/4', 4, DOC_DATA)]); + changedDocs = await readerCache.getNewDocumentChanges(); + assertMatches([doc('a/4', 4, DOC_DATA)], changedDocs); + }); + genericRemoteDocumentCacheTests(); }); @@ -217,7 +266,7 @@ function genericRemoteDocumentCacheTests(): void { doc('a/1', 3, DOC_DATA) ]); - let changedDocs = await cache.getNextDocumentChanges(); + let changedDocs = await cache.getNewDocumentChanges(); assertMatches( [ doc('a/1', 3, DOC_DATA), @@ -228,12 +277,12 @@ function genericRemoteDocumentCacheTests(): void { ); await cache.addEntries([doc('c/1', 3, DOC_DATA)]); - changedDocs = await cache.getNextDocumentChanges(); + changedDocs = await cache.getNewDocumentChanges(); assertMatches([doc('c/1', 3, DOC_DATA)], changedDocs); }); it('can get empty changes', async () => { - const changedDocs = await cache.getNextDocumentChanges(); + const changedDocs = await cache.getNewDocumentChanges(); assertMatches([], changedDocs); }); @@ -245,7 +294,7 @@ function genericRemoteDocumentCacheTests(): void { ]); await cache.removeEntry(key('a/2')); - const changedDocs = await cache.getNextDocumentChanges(); + const changedDocs = await cache.getNewDocumentChanges(); assertMatches( [doc('a/1', 1, DOC_DATA), removedDoc('a/2'), doc('a/3', 3, DOC_DATA)], changedDocs diff --git a/packages/firestore/test/unit/local/test_remote_document_cache.ts b/packages/firestore/test/unit/local/test_remote_document_cache.ts index 531f69e3301..04a1467f70a 100644 --- a/packages/firestore/test/unit/local/test_remote_document_cache.ts +++ b/packages/firestore/test/unit/local/test_remote_document_cache.ts @@ -15,6 +15,7 @@ */ import { Query } from '../../../src/core/query'; +import { IndexedDbRemoteDocumentCache } from '../../../src/local/indexeddb_remote_document_cache'; import { Persistence } from '../../../src/local/persistence'; import { RemoteDocumentCache } from '../../../src/local/remote_document_cache'; import { DocumentMap, MaybeDocumentMap } from '../../../src/model/collections'; @@ -67,13 +68,42 @@ export class TestRemoteDocumentCache { ); } - getNextDocumentChanges(): Promise { + getNewDocumentChanges(): Promise { return this.persistence.runTransaction( - 'getNextDocumentChanges', + 'getNewDocumentChanges', 'readonly', txn => { return this.cache.getNewDocumentChanges(txn); } ); } + + resetLastProcessDocumentChange(): Promise { + return this.persistence.runTransaction( + 'resetLastProcessDocumentChange', + 'readonly', + txn => { + return this.cache.resetLastProcessedDocumentChange(txn); + } + ); + } + + removeDocumentChangesThroughChangeId(changeId: number): Promise { + if (!(this.cache instanceof IndexedDbRemoteDocumentCache)) { + throw new Error( + 'Can only removeDocumentChangesThroughChangeId() in IndexedDb' + ); + } + return this.persistence.runTransaction( + 'removeDocumentChangesThroughChangeId', + 'readwrite-primary', + txn => { + return (this + .cache as IndexedDbRemoteDocumentCache).removeDocumentChangesThroughChangeId( + txn, + changeId + ); + } + ); + } } diff --git a/packages/firestore/tslint.json b/packages/firestore/tslint.json index ecd6ae6b247..c27f046465d 100644 --- a/packages/firestore/tslint.json +++ b/packages/firestore/tslint.json @@ -25,7 +25,7 @@ "interface-name": [true, "never-prefix"], "jsdoc-format": true, "label-position": true, - "max-line-length": [true, {"limit": 100, "ignore-pattern": "https?://"}], + "max-line-length": [true, {"limit": 100, "ignore-pattern": "^import|https?://"}], "member-access": [true, "no-public"], "new-parens": true, "no-angle-bracket-type-assertion": true, From 8be24490fcaf4121004e3f4f46ad7f879a8f7b42 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 3 Oct 2018 10:27:16 -0700 Subject: [PATCH 2/5] Review feedback --- packages/firestore/src/core/sync_engine.ts | 4 +-- .../local/indexeddb_remote_document_cache.ts | 31 +++++++++---------- packages/firestore/src/local/local_store.ts | 10 ------ .../src/local/memory_remote_document_cache.ts | 7 ----- .../src/local/remote_document_cache.ts | 15 +++------ .../unit/local/remote_document_cache.test.ts | 11 +++---- .../unit/local/test_remote_document_cache.ts | 26 ++++------------ 7 files changed, 30 insertions(+), 74 deletions(-) diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index 2395375f332..3843d7b07c8 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -42,7 +42,7 @@ import { SortedMap } from '../util/sorted_map'; import { isNullOrUndefined } from '../util/types'; import { isPrimaryLeaseLostError } from '../local/indexeddb_persistence'; -import { isRemoteDocumentChangesGarbageCollectedError } from '../local/indexeddb_remote_document_cache'; +import { isDocumentChangeMissingError } from '../local/indexeddb_remote_document_cache'; import { ClientId, SharedClientState } from '../local/shared_client_state'; import { QueryTargetState, @@ -1016,7 +1016,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer { ); }, async err => { - if (isRemoteDocumentChangesGarbageCollectedError(err)) { + if (isDocumentChangeMissingError(err)) { const activeTargets: TargetId[] = []; objUtils.forEachNumber(this.queryViewsByTarget, target => activeTargets.push(target) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 5dc9e052f08..2d8c749e654 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -41,7 +41,7 @@ import { PersistencePromise } from './persistence_promise'; import { RemoteDocumentCache } from './remote_document_cache'; import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db'; -const REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG = +const REMOTE_DOCUMENT_CHANGE_MISSING_ERR_MSG = 'The remote document changelog no longer contains all changes for all ' + 'local query views. It may be necessary to rebuild these views.'; @@ -171,16 +171,22 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { ); let firstIteration = true; - return documentChangesStore(transaction) + const changesStore = documentChangesStore(transaction); + return changesStore .iterate({ range }, (_, documentChange) => { if (firstIteration) { // If our client was throttled for more than 30 minutes, another // client may have garbage collected the remote document changelog. if (this._lastProcessedDocumentChangeId + 1 !== documentChange.id) { - return PersistencePromise.reject( - new FirestoreError( - Code.DATA_LOSS, - REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG + // Reset the `lastProcessedDocumentChangeId` to allow further + // invocations to successfully return the changes after this + // rejection. + return this.synchronizeLastDocumentChangeId(changesStore).next(() => + PersistencePromise.reject( + new FirestoreError( + Code.DATA_LOSS, + REMOTE_DOCUMENT_CHANGE_MISSING_ERR_MSG + ) ) ); } @@ -221,13 +227,6 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { return documentChangesStore(transaction).delete(range); } - resetLastProcessedDocumentChange( - transaction: PersistenceTransaction - ): PersistencePromise { - const store = documentChangesStore(transaction); - return this.synchronizeLastDocumentChangeId(store); - } - private synchronizeLastDocumentChangeId( documentChangesStore: SimpleDbStore< DbRemoteDocumentChangesKey, @@ -247,12 +246,10 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { } } -export function isRemoteDocumentChangesGarbageCollectedError( - err: FirestoreError -): boolean { +export function isDocumentChangeMissingError(err: FirestoreError): boolean { return ( err.code === Code.DATA_LOSS && - err.message === REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG + err.message === REMOTE_DOCUMENT_CHANGE_MISSING_ERR_MSG ); } diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index d6a3a81205d..3ca9cecbee6 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -869,14 +869,4 @@ export class LocalStore { } ); } - - resetLastDocumentChangeId(): Promise { - return this.persistence.runTransaction( - 'Reset last document change ID', - 'readonly', - txn => { - return this.remoteDocuments.resetLastProcessedDocumentChange(txn); - } - ); - } } diff --git a/packages/firestore/src/local/memory_remote_document_cache.ts b/packages/firestore/src/local/memory_remote_document_cache.ts index bee25036aa0..ad945b68b05 100644 --- a/packages/firestore/src/local/memory_remote_document_cache.ts +++ b/packages/firestore/src/local/memory_remote_document_cache.ts @@ -99,11 +99,4 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache { return PersistencePromise.resolve(changedDocs); } - - resetLastProcessedDocumentChange( - transaction: PersistenceTransaction - ): PersistencePromise { - this.newDocumentChanges = documentKeySet(); - return PersistencePromise.resolve(); - } } diff --git a/packages/firestore/src/local/remote_document_cache.ts b/packages/firestore/src/local/remote_document_cache.ts index f22b4cb0b7d..17b87059e2d 100644 --- a/packages/firestore/src/local/remote_document_cache.ts +++ b/packages/firestore/src/local/remote_document_cache.ts @@ -89,20 +89,13 @@ export interface RemoteDocumentCache { * Returns the set of documents that have been updated since the last call. * If this is the first call, returns the set of changes since client * initialization. + * + * If the changelog was garbage collected and can no longer be replayed, + * `getNewDocumentChanges` will reject the returned Promise. Further + * invocations will return document changes since the point of rejection. */ // PORTING NOTE: This is only used for multi-tab synchronization. getNewDocumentChanges( transaction: PersistenceTransaction ): PersistencePromise; - - /** - * Skips all existing change log entries in IndexedDb and moves the change log - * cursor past the last existing change. This method should only be called if - * `getNewDocumentChanges()`can no longer replay all changes and all views - * have already been manually synchronized. - */ - // PORTING NOTE: This is only used for multi-tab synchronization. - resetLastProcessedDocumentChange( - transaction: PersistenceTransaction - ): PersistencePromise; } diff --git a/packages/firestore/test/unit/local/remote_document_cache.test.ts b/packages/firestore/test/unit/local/remote_document_cache.test.ts index f122058ff5e..f42fa33b615 100644 --- a/packages/firestore/test/unit/local/remote_document_cache.test.ts +++ b/packages/firestore/test/unit/local/remote_document_cache.test.ts @@ -30,7 +30,7 @@ import { import { IndexedDbRemoteDocumentCache, - isRemoteDocumentChangesGarbageCollectedError + isDocumentChangeMissingError } from '../../../src/local/indexeddb_remote_document_cache'; import { DbRemoteDocumentChanges, @@ -155,14 +155,11 @@ describe('IndexedDbRemoteDocumentCache', () => { .getNewDocumentChanges() .then( () => fail('Missing expected error'), - err => - expect(isRemoteDocumentChangesGarbageCollectedError(err)).to.be.ok + err => expect(isDocumentChangeMissingError(err)).to.be.ok ); - // Recover by moving moving the change log cursor to the last existing - // entry. We assume that as part of the recovery, all relevant changes have - // been processed manually (see `SyncEngine.applyTargetState`). - await readerCache.resetLastProcessDocumentChange(); + // Ensure that we can retrieve future changes after the we processed the + // error await writerCache.addEntries([doc('a/4', 4, DOC_DATA)]); changedDocs = await readerCache.getNewDocumentChanges(); assertMatches([doc('a/4', 4, DOC_DATA)], changedDocs); diff --git a/packages/firestore/test/unit/local/test_remote_document_cache.ts b/packages/firestore/test/unit/local/test_remote_document_cache.ts index 04a1467f70a..7f70399df3b 100644 --- a/packages/firestore/test/unit/local/test_remote_document_cache.ts +++ b/packages/firestore/test/unit/local/test_remote_document_cache.ts @@ -78,31 +78,17 @@ export class TestRemoteDocumentCache { ); } - resetLastProcessDocumentChange(): Promise { - return this.persistence.runTransaction( - 'resetLastProcessDocumentChange', - 'readonly', - txn => { - return this.cache.resetLastProcessedDocumentChange(txn); - } - ); - } - removeDocumentChangesThroughChangeId(changeId: number): Promise { - if (!(this.cache instanceof IndexedDbRemoteDocumentCache)) { - throw new Error( - 'Can only removeDocumentChangesThroughChangeId() in IndexedDb' - ); - } return this.persistence.runTransaction( 'removeDocumentChangesThroughChangeId', 'readwrite-primary', txn => { - return (this - .cache as IndexedDbRemoteDocumentCache).removeDocumentChangesThroughChangeId( - txn, - changeId - ); + if (!(this.cache instanceof IndexedDbRemoteDocumentCache)) { + throw new Error( + 'Can only removeDocumentChangesThroughChangeId() in IndexedDb' + ); + } + return this.cache.removeDocumentChangesThroughChangeId(txn, changeId); } ); } From 946ed1a7231ba9a2f3e9cf5699e0aab533b26e7d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 3 Oct 2018 10:55:08 -0700 Subject: [PATCH 3/5] Fix compile --- packages/firestore/src/core/sync_engine.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index 3843d7b07c8..a0b1fefc7c9 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -1024,7 +1024,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer { await this.synchronizeQueryViewsAndRaiseSnapshots( activeTargets ); - await this.localStore.resetLastDocumentChangeId(); } else { throw err; } From d5ba267e1e1ca7f00a59c5665b5a0fc95a66e67c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 3 Oct 2018 13:13:42 -0700 Subject: [PATCH 4/5] Don't use the escaped persistence keys when we write to Local Storage --- .../src/local/shared_client_state.ts | 31 ++++++------------- .../unit/local/persistence_test_helpers.ts | 2 +- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/packages/firestore/src/local/shared_client_state.ts b/packages/firestore/src/local/shared_client_state.ts index 3937ec598aa..b794d83e520 100644 --- a/packages/firestore/src/local/shared_client_state.ts +++ b/packages/firestore/src/local/shared_client_state.ts @@ -526,7 +526,6 @@ export class WebStorageSharedClientState implements SharedClientState { private readonly sequenceNumberKey: string; private readonly activeClients: { [key: string]: ClientState } = {}; private readonly storageListener = this.handleWebStorageEvent.bind(this); - private readonly escapedPersistenceKey: string; private readonly onlineStateKey: string; private readonly clientStateKeyRe: RegExp; private readonly mutationBatchKeyRe: RegExp; @@ -543,7 +542,7 @@ export class WebStorageSharedClientState implements SharedClientState { constructor( private readonly queue: AsyncQueue, private readonly platform: Platform, - persistenceKey: string, + private readonly persistenceKey: string, private readonly localClientId: ClientId, initialUser: User ) { @@ -555,7 +554,7 @@ export class WebStorageSharedClientState implements SharedClientState { } // Escape the special characters mentioned here: // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions - this.escapedPersistenceKey = persistenceKey.replace( + const escapedPersistenceKey = persistenceKey.replace( /[.*+?^${}()|[\]\\]/g, '\\$&' ); @@ -565,26 +564,20 @@ export class WebStorageSharedClientState implements SharedClientState { this.localClientStorageKey = this.toWebStorageClientStateKey( this.localClientId ); - this.sequenceNumberKey = `${SEQUENCE_NUMBER_KEY_PREFIX}_${ - this.escapedPersistenceKey - }`; + this.sequenceNumberKey = `${SEQUENCE_NUMBER_KEY_PREFIX}_${persistenceKey}`; this.activeClients[this.localClientId] = new LocalClientState(); this.clientStateKeyRe = new RegExp( - `^${CLIENT_STATE_KEY_PREFIX}_${this.escapedPersistenceKey}_([^_]*)$` + `^${CLIENT_STATE_KEY_PREFIX}_${escapedPersistenceKey}_([^_]*)$` ); this.mutationBatchKeyRe = new RegExp( - `^${MUTATION_BATCH_KEY_PREFIX}_${ - this.escapedPersistenceKey - }_(\\d+)(?:_(.*))?$` + `^${MUTATION_BATCH_KEY_PREFIX}_${escapedPersistenceKey}_(\\d+)(?:_(.*))?$` ); this.queryTargetKeyRe = new RegExp( - `^${QUERY_TARGET_KEY_PREFIX}_${this.escapedPersistenceKey}_(\\d+)$` + `^${QUERY_TARGET_KEY_PREFIX}_${escapedPersistenceKey}_(\\d+)$` ); - this.onlineStateKey = `${ONLINE_STATE_KEY_PREFIX}_${ - this.escapedPersistenceKey - }`; + this.onlineStateKey = `${ONLINE_STATE_KEY_PREFIX}_${persistenceKey}`; // Rather than adding the storage observer during start(), we add the // storage observer during initialization. This ensures that we collect @@ -926,22 +919,18 @@ export class WebStorageSharedClientState implements SharedClientState { `Client key cannot contain '_', but was '${clientId}'` ); - return `${CLIENT_STATE_KEY_PREFIX}_${ - this.escapedPersistenceKey - }_${clientId}`; + return `${CLIENT_STATE_KEY_PREFIX}_${this.persistenceKey}_${clientId}`; } /** Assembles the key for a query state in WebStorage */ private toWebStorageQueryTargetMetadataKey(targetId: TargetId): string { - return `${QUERY_TARGET_KEY_PREFIX}_${ - this.escapedPersistenceKey - }_${targetId}`; + return `${QUERY_TARGET_KEY_PREFIX}_${this.persistenceKey}_${targetId}`; } /** Assembles the key for a mutation batch in WebStorage */ private toWebStorageMutationBatchKey(batchId: BatchId): string { let mutationKey = `${MUTATION_BATCH_KEY_PREFIX}_${ - this.escapedPersistenceKey + this.persistenceKey }_${batchId}`; if (this.currentUser.isAuthenticated()) { diff --git a/packages/firestore/test/unit/local/persistence_test_helpers.ts b/packages/firestore/test/unit/local/persistence_test_helpers.ts index 6a0eccbb4ac..a52d7105720 100644 --- a/packages/firestore/test/unit/local/persistence_test_helpers.ts +++ b/packages/firestore/test/unit/local/persistence_test_helpers.ts @@ -56,7 +56,7 @@ export const INDEXEDDB_TEST_DATABASE_ID = new DatabaseId('test-project'); /** The DatabaseInfo used by most tests that access IndexedDb. */ const INDEXEDDB_TEST_DATABASE_INFO = new DatabaseInfo( INDEXEDDB_TEST_DATABASE_ID, - 'PersistenceTestHelpers', + '[PersistenceTestHelpers]', 'host', /*ssl=*/ false ); From 20f4d50b5a524ec07959b919d6b1e1768ab1a5e9 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 3 Oct 2018 13:59:52 -0700 Subject: [PATCH 5/5] Set firstIteration to false --- .../firestore/src/local/indexeddb_remote_document_cache.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 2d8c749e654..ba10a899bf0 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -175,6 +175,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { return changesStore .iterate({ range }, (_, documentChange) => { if (firstIteration) { + firstIteration = false; + // If our client was throttled for more than 30 minutes, another // client may have garbage collected the remote document changelog. if (this._lastProcessedDocumentChangeId + 1 !== documentChange.id) { @@ -190,7 +192,6 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache { ) ); } - firstIteration = false; } changedKeys = changedKeys.unionWith(