Handle IndexedDB errors in unsubscribe callback #3162
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
Addresses one of the three problems I found during randomized testing.
schmidt-sebastian
force-pushed
the
mrschmidt/unsubscribe
branch
from
6c37162
to
9686261
Compare
| try { | ||
| await this.eventManager.unlisten(eventEmitter!); | ||
| const currentEventEmitter = this.queryListeners.get(query); | ||
| // Before removing the listener, verify that the query hasn't been |
There was a problem hiding this comment.
I must be missing some context for this comment. If a user attempts to unlisten, that fails, gets in the retry queue, and then they listen to the same query while we're retrying, doesn't the event manager have two listeners for that query? The first query will stil succeed in unlistening.
There was a problem hiding this comment.
queryListeners is a map of active listeners that the Spec tests care about. This code here was my first attempt at making sure that we don't remove a query from the Spec tests view if another operation has re-registered it. I was able to simplify this though by removing the query directly and outside of the queue, which should have no effect on the spec tests since this step would have previously executed in a very similar order.
| @@ -404,7 +404,7 @@ export class FirestoreClient { | |||
| if (this.clientTerminated) { | |||
| return; | |||
| } | |||
| this.asyncQueue.enqueueAndForget(() => { | |||
| this.asyncQueue.enqueueRetryable(() => { | |||
There was a problem hiding this comment.
Why is this necessary? I thought that if we failed we tore down the streams? At that point haven't we effectively unlistened anyway? All that's missing is updating our in-memory bookkeeping to indicate that we don't want to restart the target when IndexedDB becomes available again.
One other concern is that if IndexedDB is failing we've likely been put in the background and may not come back for a while--possibly more than the 30 minutes for which a resume token means free query resumption. If we were asleep that long the user would be charged for the query anew--for a query they were trying to stop listening to. If all we're losing is the resume token and the sequence numbers for exact GC ordering, it seems better to let the unlisten succeed despite failing to write.
So in the primary tab, if we fail trying to save metadata data about unlistening that's not really a big deal. Can a secondary tab fail trying to tell the primary that it wants to unlisten? In that case we should probably retry.
If this squares with your understanding, it seems like this line may actually be correct: we do want to retry unlistens but only when it's the secondary tab asking the primary tab to do it. The part that's missing is that primary tabs should handle IndexedDB errors and succeed.
There was a problem hiding this comment.
This code path calls into LocalStore to invoke "Release Target" and then calls RemoteStore. We could change the plumbing here to drive this from RemoteStore, in which case RemoteStore could go offline and do its own retry. That sounds a like a fair bit of code churn that may lead to some unexpected side effects - the mapping between the lastListen state in EventManager and the active list of targets in SyncEngine/LocalStore may go out of sync.
One easy thing we could do here is just swallow IndexedDB failures if "Release Target" fails, which would even remove some of the newly added error handling in RemoteStore. Do you think that's worth exploring?
There was a problem hiding this comment.
just swallow IndexedDB failures if "Release Target" fails
This is what I was suggesting: releasing a target in the LocalStore is just about making sure the final updates are made for GC metadata (and the in-memory tracking of the target). It may be that even LocalStore itself may need to change so that even if the transaction fails it will remove the target from its own set of live things.
I'm not sure this undoes the error handling in RemoteStore, since it's really only unsubscribe can work this way.
There was a problem hiding this comment.
I updated the PR to ignore errors in "Release target". Unfortunately, since a failed Target can generate a "fake" Remote Event (for Limbo resolutions), the code path that calls into this from RemoteStore still has to use the IndexedDB fallback.
🦋 Changeset is good to goLatest commit: 1ba0fe1 We got this. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
schmidt-sebastian
requested review from
alikn,
andirayo,
avolkovi,
egilmorez,
erikeldridge,
sam-gc,
scottcrossen,
yuchenshi,
zijianjoy and
zwu52
as code owners
schmidt-sebastian
changed the title
* Clean up unused FCM secret * Update breezy-queens-give.md Co-authored-by: Feiyang <feiyangc@google.com>
* change to ts * use the correct file name * remove -exp from types packages when releasing * werid hack to make exp release build * skip tests in exp release * do not run test * revert changes made for testing * remove extraneous build target * Create warm-suns-dream.md
* Make onSnapshot work with rxjs observers * Create thin-ligers-fold.md * Update thin-ligers-fold.md
* Update dependency firebase-tools to v8 * Create yellow-lamps-greet.md Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Feiyang <feiyangc@google.com>
* point to the typing file in functions-exp * use app-exp as the service name in functions * Create chilled-beers-chew.md
* Update dependency eslint to v7 * Create two-weeks-thank.md Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Feiyang <feiyangc@google.com>
schmidt-sebastian
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly. In order to pass this check, please resolve this problem and then comment ℹ️ Googlers: Go here for more info. |
| } catch (e) { | ||
| if (isIndexedDbTransactionError(e)) { | ||
| // If `releaseTarget` fails, we did not advance the sequence number of | ||
| // the target. While the target might be deleted earlier than it |
There was a problem hiding this comment.
Optional: since the degree of "earlier" is very slight (we already update the sequence number when you start listening and during updates so this is only the final update on unlisten), consider softening the wording here or giving more context. Suggestion:
All
releaseTargetdoes is record the final metadata state for the target, but we've been recording this periodically during target activity. If we lose this write this could cause a very slight difference in the order of target deletion during GC, but we don't define exact LRU semantics anyway so this is acceptable.
There was a problem hiding this comment.
Done. Thanks for the rewrite :)
Addresses one of the three problems I found during randomized testing.
Addresses #2755