Skip to content

Ignore IndexedDB failure during lease refresh #3020

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 14, 2020
Merged

Ignore IndexedDB failure during lease refresh #3020

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3073f60
Ignore IndexedDB failure during lease refresh
schmidt-sebastian May 5, 2020
d87dc1e
Rewrite everything
schmidt-sebastian May 6, 2020
8d08444
Review
schmidt-sebastian May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/firestore/src/local/indexeddb_persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,16 @@ export class IndexedDbPersistence implements Persistence {
})
.catch(e => {
if (!this.allowTabSynchronization) {
throw e;
}
if (e.name === 'IndexedDbTransactionError') {
logDebug(LOG_TAG, "Failed to extend owner lease: ", e);
// Proceed in primary mode since the client was not initialized
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is proceeding here assuming we're the primary a useful thing?

When this method is called in the course of enablePersistence, don't we want to report a failure to open the database?

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to let start() proceed if this check failed. I have now changed the code to fail start() (and hence enablePersistence()) if we get an IndexedDbTransactionError and synchronizeTabs is false.
The fix for this was pretty straightforward, but I ended up changing the callsites to use this.runTransaction instead of this.simpleDb.runTransaction, as this allows me to use MockPersistence to inject failures.

Note that the behavior of this.runTransaction is not all that different: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/local/indexeddb_persistence.ts#L808
We make one additional call to verify the lease (which succeeds if no other client has exclusive access). We also obtain access to all ObjectStores, but I think that is not all that different since there is no operation that doesn't also access the DbPrimaryStore.

// to support multi-tab. Any subsequent access to IndexedDB will
// verify the lease.
return true;
} else {
throw e;
}
}

logDebug(
LOG_TAG,
Expand Down