Don't crash if write cannot be persisted #2938
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
|
Does this also address the issue if doing batched writes? |
Yes. |
| userCallback.reject( | ||
| new FirestoreError( | ||
| Code.UNKNOWN, | ||
| 'Query allocation failed with environment error: ' + e |
There was a problem hiding this comment.
"Query allocation" seems like the wrong description of this error. Also, since this is catching everything, this isn't necessarily something to do with the environment. "Failed to enqueue a write in the local database"?
The catch here is also overbroad: this will catch internal assertion errors and the like as well. Taking an approach like the one I suggested in #2919 would make it possible to catch just errors related to IndexedDB.
Also, once we know the error is specifically about IndexedDB, I think the code can probably become DATA_LOSS. Or maybe UNAVAILABLE? DATA_LOSS is probably a bad choice because it is a permanent error.
There was a problem hiding this comment.
Sorry, "Query allocation" was the error message for my next PR.
I updated the PR with the changes in #2919 and only throw this error for IndexedDbTransactionErrors. I am a little uncertain whether we should just re-throw the error here or wrap it. For now, I decided to wrap it in a FirestoreError, as this makes it easier for our users to share error handling logic between the various backend errors and the new IndexedDB error.
| result = await this.localStore.localWrite(batch); | ||
| } catch (e) { | ||
| // If we can't persist the mutation, we reject the user callback and don't | ||
| // send the mutation. The user can then retry the write. We currently do |
There was a problem hiding this comment.
This extra bit about not using enqueueRetryable isn't all that helpful. We're failing user writes because that's the developer experience we want users of the Firestore SDK to have. If we thought it was a better experience not to fail writes we'd make the changes required to make that happen.
There was a problem hiding this comment.
I removed it.
| @@ -75,5 +75,61 @@ describeSpec( | |||
| .expectEvents(query, {}); | |||
| } | |||
| ); | |||
|
|
|||
| specTest('Recovers when write cannot be persisted', [], () => { | |||
There was a problem hiding this comment.
Do these tests need a no-ios and no-android tag or some new kind of tag to indicate that failure recovery is required?
There was a problem hiding this comment.
All tests in this file are marked no-ios and no-android at the moment: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/test/unit/specs/recovery_spec.test.ts#L26
There was a problem hiding this comment.
FYI: I pushed a commit to this PR to remove the durable-persistence tags as the new recovery tests also work with memory persistence. Most of my changes in my next PR were due to the formatting caused by this change.
Addresses #2755