Ignore IndexedDB failure during lease refresh #3020
Conversation
Binary Size ReportAffected SDKs
Test Logs |
| } | ||
| if (e.name === 'IndexedDbTransactionError') { | ||
| logDebug(LOG_TAG, "Failed to extend owner lease: ", e); | ||
| // Proceed in primary mode since the client was not initialized |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
schmidt-sebastian
force-pushed
the
mrschmidt/leaserefreshv2
branch
from
09ec0ee to
d87dc1e
Compare
| // client startup, we reject the Promise returned by | ||
| // `enablePersistence()` and the user can continue to use Firestore | ||
| // with in-memory persistence. | ||
| // If this fails during a lease refresh, we will instead block the |
There was a problem hiding this comment.
It seems like removing this error here is a regression. Consider:
- If the client successfully starts
- goes into the background and loses access to IndexedDB
- stays in the background for longer than the lease duration
If device comes back online the user opens another tab before our retry has succeeded in refreshing the lease, there's a very real chance we'll lose this race. Users don't even have to be mixing and matching modes for this to fail now.
There was a problem hiding this comment.
Summary of my last 5 minutes:
- Gil is wrong.
- Gil is right.
- Gil is wrong.
- Gil is right.
- Gil it right and wrong.
The idea behind the lease refresh was it would be a "best effort" only operation. If if fails or stalls, any operation to IndexedDB should verify the expected state. That is:
- "readwrite-primary" operations should verify that the tab has the lease or can become the leaseholder. This already happens.
- "readwrite", "readonly" operations should verify that they are allowed to access IndexedDB (i.e. there is no primary tab without
synchronizeTabsturned on). This is not happening right now. Instead, this is periodically verified via the code that I removed.
So, in conclusion: We should move this verification and run it as part of every transaction. I will leave the code in master and work on this in a separate PR.
Adresses #2755