Skip to content
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.

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

Retry IDB errors from batch.commit #325

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nVitius
Copy link

@nVitius nVitius commented Apr 28, 2020

This addresses #321

I am a little concerned that, due to the nature of the issue in IDB, an immediate single retry might not work in some cases.

The original issue in Firestore was caused when the IDB process was force-closed by the system (e.g. when a mobile device goes to sleep) and the transaction would be aborted. It might be possible that the retry happens before Firebase has re-established an IDB connection and is failed again.

To my knowledge, there's no easy way to test this. I think we can assume that if a commit is rejected with this error from Firebase, all subsequent commits will be rejected until it comes back online. Perhaps it might be sane to implement a retry with exponential back-off for this error.

@nVitius
Copy link
Author

nVitius commented Apr 28, 2020

Also, I didn't add tests for this. I couldn't find existing tests for the batchSync method directly, and wasn't sure how to mock the batch.commit error. I am willing to write the tests if you want them and can point me in the right direction.

@mesqueeb
Copy link
Owner

@nVitius thanks for your PR.
I am a bit concerned about your message though. If an immidiate retry probably won't work any way, what is the benefit of giving special treatment to this error in the first place, opposed to just having it throw the error, and have the dev implementing the action handle the error as they please?

@mesqueeb
Copy link
Owner

PS: were you gonna add a special message for firestore-idb-unrecoverable in the error logger?

@nVitius
Copy link
Author

nVitius commented Apr 29, 2020

@mesqueeb You are right. The more I think about it, the less straight-forward dealing with this error becomes... Originally, I thought Firebase would recover itself and allow for a retry. I've been reading through the code today though, and I don't think that is actually the case. If we can't retry here, then the client will just have to handle the error.

I wonder if we could add a way to register error handlers for easy-firestore actions. It would be nice to provide the client with a way to handle the error recovery in one place, as opposed to every time an action is called. What do you think?

I made an issue on the Firebase repo to clear up this question - firebase/firebase-js-sdk#2997

@mesqueeb
Copy link
Owner

@nVitius based on the message from the Google employee: Sounds to me like firebase SDK will be the one that retries. So we don't need to do anything? Let me know how you interpret the message.

@nVitius
Copy link
Author

nVitius commented May 2, 2020

@mesqueeb It sounds like they are already retrying for some events, but will not be for new writes and onSnapshot listeners. So new writes will still require the client to retry. I think we should move ahead with this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants