Make 'handleClientStateEvent()/handleQueryTargetEvent()' idempotent #2916
Conversation
schmidt-sebastian
changed the title
This solves a problem where we were able to modify a in-memory state before the database transaction failed and also addresses issues where targets to allocate already existed or rejected targets were missing. I beliuve there are no other issues in the WebStorage callbacks
schmidt-sebastian
force-pushed
the
mrschmidt/idempotenthandleclientstate
branch
from
bd4da5f to
e7fc1d3
Compare
Binary Size ReportAffected SDKs
Test Logs |
| // the database. | ||
| .recoverDatabase() | ||
| .runTimer(TimerId.AsyncQueueRetry) | ||
| .expectActiveTargets() |
There was a problem hiding this comment.
Should we expect that there is an active target before recoverDatabase?
There was a problem hiding this comment.
The targets are checked at each step, but adding this here explicitly makes it much clearer what is going on. Done.
| `Tried to release nonexistent target: ${targetId}` | ||
| ); | ||
|
|
||
| if (targetData === null) { |
There was a problem hiding this comment.
How would this happen in practice that isn't a bug? I mean, how could we get in a state where the sync engine thought we were listening but local store didn't?
There was a problem hiding this comment.
I was trying to guard against this line: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/core/sync_engine.ts#L1115
I did miss that this is only executed if the target is still active. I reverted this change.
| // The primary client 0 receives a notification that the query can | ||
| // be released, but it can only process the change after we recover | ||
| // the database. | ||
| .expectActiveTargets({ query}) |
There was a problem hiding this comment.
nit: should there be a space between query and }?
There was a problem hiding this comment.
Yes. As you may have noticed, the prettier pre-commit hook doesn't work for me at all. I have been working with @hsubox76 to see if this can be fixed, but it seems like the behavior I am seeing is actually the indented behavior. Pre-commit hooks are only meant to push changes that existed prior to running.
This solves a problem where we were able to modify a in-memory state before the database transaction failed and also addresses issues where targets to allocate already existed or rejected targets were missing.
I believe there are no other issues in the WebStorage callbacks.
Addresses #2755