IndexedDB recovery for waitForPendingWrites() #3038
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/pendingwrites
branch
from
649ed1d to
0852c5d
Compare
Binary Size ReportAffected SDKs
Test Logs |
| const query = listener.query; | ||
| let firstListen = false; | ||
| listen(listener: QueryListener): Promise<void> { | ||
| return executeWithIndexedDbRecovery( |
There was a problem hiding this comment.
I can't say this pattern is really different from try/catch, except it's hiding that.
What do you think of the more direct:
try {
const query = listener.query;
//
} catch (e) {
const msg = `Initialization of query '${query}' failed: ${e}`;
logError(LOG_TAG, msg);
throwIfUnrecoverable(e);
listener.onError(new FirestoreError(Code.UNAVAILABLE, msg));
}As I see it:
- It's not really more code than what you have here
- It uses async/await try/cache directly and looks more natural/synchronous-like
- On the flip side, there aren't a series of callbacks that you have to remember their meaning.
- There isn't an extra log message about "Internal operation failed"
- It still abstracts the handling of the
IndexedDbTransactionErrortype
Building on this, you could actually cut the verbosity of each call down by making a checkRecoverable that takes the message, logs it, and then throws if unrecoverable or returns the FirestoreError if it is?
The catch blocks would look like:
const wrapped = checkRecoverable(e, `Initialization of query '${query}' failed: ${e}`);
listener.onError(new FirestoreError(Code.UNAVAILABLE, msg));The implementation would look like:
checkRecoverable(e: Error, message: string): FirestoreError {
logError(LOG_TAG, msg);
if (e.name == ....) {
return new FirestoreError(unavailable, message);
} else {
throw e;
}
}WDYT?
There was a problem hiding this comment.
I tried really hard to somehow push the try/catch into my helper, but it might have been a bit too mush. I updated the PR to use a helper that returns a FirestoreError if the error is an IndexedDB error, otherwise it throws.
| } | ||
| } | ||
| await this.scheduleGC(localStore); | ||
| return executeWithIndexedDbRecovery( |
There was a problem hiding this comment.
In particular, this change has fairly significantly harmed this method:
- Why is the extra
.then(() => {})required? - Going back to
then/catchfromasync/awaitis a bummer.
There was a problem hiding this comment.
This is now reverted, but there are some arguments for using Promise chains here:
- In ES5 code, these are shorter since the function doesn't have to be wrapped in a generator. We haven't really optimized for this in other cases and the overhead might be outweighed by readability.
- The Promise version doesn't re-schedule the GC task if we have lost the lease. As pointed out in the previous PR, this is also not really an issue though since we stop the scheduler during the next lease refresh.
| @@ -1144,7 +1144,7 @@ describe('IndexedDb: allowTabSynchronization', () => { | |||
| 'clientA', | |||
| /* multiClient= */ false, | |||
| async db => { | |||
| db.injectFailures = true; | |||
| db.injectFailures = { updateClientMetadataAndTryBecomePrimary: true }; | |||
There was a problem hiding this comment.
I think these tests are now broken in master because I merged two conflicting PRs that were both passing all tests on their own.
This:
executeWithIndexedDbRecoveryfunction that aims to centralize the error checking for IndexedDbTransactionErrors. I wasted a lot of time trying to come up with some clean API and clever API and have concluded that the current approach is the most useable (unless we want to keep current duplication).waitForPendingWrites()Promise if we cannot access IndexedDb. Note that we cannot retry here, since the retry might let other writes "slip through" which would then also erroneously be awaited.Addresses #2755