[Multi-Tab] Detect and recover from GCed Remote Document Changelog #1272
Conversation
schmidt-sebastian
requested review from
gsoltis,
mikelehen,
rsgowman,
var-const,
wilhuff and
zxu123
as code owners
schmidt-sebastian
force-pushed
the
mrschmidt-recover
branch
from
3a110a5 to
e6b0768
Compare
| * have already been manually synchronized. | ||
| */ | ||
| // PORTING NOTE: This is only used for multi-tab synchronization. | ||
| resetLastProcessedDocumentChange( |
There was a problem hiding this comment.
"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.
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.
I removed the method and inlined the call that resets the change log pointer.
| ) | ||
| ); | ||
| } | ||
| firstIteration = false; |
There was a problem hiding this comment.
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.
Consider it done.
| } | ||
| } | ||
|
|
||
| export function isRemoteDocumentChangesGarbageCollectedError( |
There was a problem hiding this comment.
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:
export const DOCUMENT_CHANGE_MISSING_ERROR = new Error(...);
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.
I changed the name to isDocumentChangeMissingError.
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.
| 'readonly', | ||
| txn => { | ||
| return this.cache.getNewDocumentChanges(txn); | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| resetLastProcessDocumentChange(): Promise<void> { |
There was a problem hiding this comment.
I'm hoping this method goes away, but if not you're missing 'ed' here and below.
There was a problem hiding this comment.
And it's gone!
| 'readwrite-primary', | ||
| txn => { | ||
| return (this | ||
| .cache as IndexedDbRemoteDocumentCache).removeDocumentChangesThroughChangeId( |
There was a problem hiding this comment.
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.
No, I managed to get rid of it by moving the assert.
| // Reset the `lastProcessedDocumentChangeId` to allow further | ||
| // invocations to successfully return the changes after this | ||
| // rejection. | ||
| return this.synchronizeLastDocumentChangeId(changesStore).next(() => |
There was a problem hiding this comment.
Just to verify, did you consider just letting this iteration continue so that line 199 can correctly update _lastProcessedDocumentChangeId, and then we could (I think?) get rid of the synchronizeLastDocumentChangeId? I think the reduced code complexity would outweigh any potential performance saving of getting to skip the changedKeys accounting...
|
I tested this PR manually this morning. I found one area of improvement (we should set firstIteration to false before we return, since the iterate function just keeps iterating). Outside of this PR, I did find a slew of things that were not working/no longer working in Multi-Tab, so thanks for asking for an extra QA run :) |
This is meant to address the case when one tab removed document changes from the document change log that a throttled tab has not yet seen. The detection relies on the fact that change IDs are incremented by 1 each time (as guaranteed by https://www.w3.org/TR/IndexedDB/#key-generator-construct)