-
Notifications
You must be signed in to change notification settings - Fork 935
[Multi-Tab] Detect and recover from GCed Remote Document Changelog #1272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
e6b0768
8be2449
946ed1a
d5ba267
dcb92e6
20f4d50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<void> { | ||
// 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<void> { | ||
const store = documentChangesStore(transaction); | ||
return this.synchronizeLastDocumentChangeId(store); | ||
} | ||
|
||
private synchronizeLastDocumentChangeId( | ||
documentChangesStore: SimpleDbStore< | ||
DbRemoteDocumentChangesKey, | ||
DbRemoteDocumentChanges | ||
> | ||
): PersistencePromise<void> { | ||
// 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like verbosity, but "is...Error" gets hard to parse when there are 5 words in between. How about isDocumentChangeMissingError()? I was also wondering if you could just:
But I am not sure how JS works. Would that mess up the stack trace in the error or something? It's also a little dubious to be using errors for control flow. We could change getNewDocumentChanges() to return null if it encountered a missing change or something, but I think I'm not that thrilled about the idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the name to In general, I would like to shy away from statically allocating errors as it does mess up the stacktraces (even in JS). I'm gonna use the current fire alarm in SFO as an excuse to not comment on the API contract. I can't really think of a much better way to convey this information. |
||
err: FirestoreError | ||
): boolean { | ||
return ( | ||
err.code === Code.DATA_LOSS && | ||
err.message === REMOTE_DOCUMENT_CHANGELOG_GARBAGE_COLLECTED_ERR_MSG | ||
); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,4 +94,15 @@ export interface RemoteDocumentCache { | |
getNewDocumentChanges( | ||
transaction: PersistenceTransaction | ||
): PersistencePromise<MaybeDocumentMap>; | ||
|
||
/** | ||
* 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "lastProcessedDocumentChange" is using implementation terminology and should instead use the same terminology as the rest of this interface, which probably means something like "resetDocumenChanges()" (to match "getNewDocumentChanges()"). The comment should also be updated to be more implementation-agnostic (including not mentioning IndexedDb). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, can we get rid of this method and just have getNewDocumentChanges() maintain its guarantee that it'll only return new changes since the last call, even if it hits the error? That is, have it still "consume" all of the new changes, updating lastProcessedChangeId_, even if there was an error? That seems sensible and simplifies the interface (and maybe our implementation?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the method and inlined the call that resets the change log pointer. |
||
transaction: PersistenceTransaction | ||
): PersistencePromise<void>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<MaybeDocumentMap> { | ||
getNewDocumentChanges(): Promise<MaybeDocumentMap> { | ||
return this.persistence.runTransaction( | ||
'getNextDocumentChanges', | ||
'getNewDocumentChanges', | ||
'readonly', | ||
txn => { | ||
return this.cache.getNewDocumentChanges(txn); | ||
} | ||
); | ||
} | ||
|
||
resetLastProcessDocumentChange(): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hoping this method goes away, but if not you're missing 'ed' here and below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And it's gone! |
||
return this.persistence.runTransaction( | ||
'resetLastProcessDocumentChange', | ||
'readonly', | ||
txn => { | ||
return this.cache.resetLastProcessedDocumentChange(txn); | ||
} | ||
); | ||
} | ||
|
||
removeDocumentChangesThroughChangeId(changeId: number): Promise<void> { | ||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this cast necessary? If you moved your if check above into this closure it probably wouldn't be... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I managed to get rid of it by moving the assert. |
||
txn, | ||
changeId | ||
); | ||
} | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my comment elsewhere, perhaps rework this so that getNewDocumentChanges() always advances past all document changes, even if it ultimately returns an error... e.g. by changing firstIteration to minChangeId and checking it in the next() block...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider it done.