search

mesqueeb / vuex-easy-firestore

Easy coupling of firestore and a vuex module. 2-way sync with 0 boilerplate!
https://mesqueeb.github.io/vuex-easy-firestore
MIT License
234 stars 28 forks source link

Retry IDB errors from batch.commit #325

Open nVitius opened 4 years ago

nVitius commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

nVitius commented 4 years ago

@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 - https://github.com/firebase/firebase-js-sdk/issues/2997

mesqueeb commented 4 years ago

@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 commented 4 years ago

@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.