Take WatchStream offline when IndexedDB is unavailable #3010
Conversation
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
changed the title
| */ | ||
| private async disableNetworkUntilRecovery(e: FirestoreError): Promise<void> { | ||
| if (e.name === 'IndexedDbTransactionError') { | ||
| ++this.indexedDbFailureBarrier; |
There was a problem hiding this comment.
This mechanism seems more complicated/fiddly than it needs to be.
If we get multiple watch change events in a row, this is going to bump this indexedDbFailureBarrier multiple times and enqueue multiple retry tasks. When IndexedDB becomes available, each of them is going to succeed, decrement the count, and try to enable the network but only the last call to enableNetworkInternal will succeed.
It seems like instead of keeping a count, we could instead keep a boolean like indexedDbFailed. Then in this method:
- If not
indexedDbFailed, set it and enqueue the retry task - If
indexedDbFailed, just log the error but do nothing. - In the retry task, if
getLastRemoteSnapshotVersionsucceeds, flipindexedDbFailedback to false and enable the network.
There was a problem hiding this comment.
You say "complicated/fiddly", I say "advanced" :)
We should only ever get one of these events, since we go offline right away and discard all further messages from the stream. I realize that this is a good argument for making this a boolean and updated the code accordingly.
| private async disableNetworkUntilRecovery(e: FirestoreError): Promise<void> { | ||
| if (e.name === 'IndexedDbTransactionError') { | ||
| ++this.indexedDbFailureBarrier; | ||
| await this.disableNetworkInternal(); |
There was a problem hiding this comment.
disableNetworkInternal does not change the onlineStateTracker, but it seems like while we've intentionally disabled the network we should mark ourselves offline, no?
There was a problem hiding this comment.
That will create some flicker, but it is the right thing to do. Updated.
There was a problem hiding this comment.
I had to add the state transition here since we don't raise offline events when disableNetworkInternal is called from shutdown or for primary state transitions.
| await this.raiseWatchSnapshot(snapshotVersion); | ||
| } | ||
| } catch (e) { | ||
| logDebug(LOG_TAG, 'Failed to raise snapshot: ' + e); |
There was a problem hiding this comment.
Is there any identifying information about the snapshot that we can log here?
There was a problem hiding this comment.
I am not sure if it will help, but I added the snapshot version.
There was a problem hiding this comment.
The snapshot version probably isn't that useful, but the target ID would be. Anything to help associate this message with its context.
If the targetId isn't available going back to what you had before is probably better.
There was a problem hiding this comment.
I changed this back to the original message. The target IDs are not easily available:
- DocumentWatchChange has
updatedTargetIdsandremovedTargetIds - ExistenceFilterChange has
targetId - WatchTargetChange has
targetIds
If I understand the flow correctly, we should always be in a WatchTargetChange to raise a snapshot, but I don't want to bake this assumption into the code for better logging. I can invest some time here, but the targets are already in the Watch response that we log right before.
| try { | ||
| await this.handleTargetError(watchChange); | ||
| } catch (e) { | ||
| logDebug(LOG_TAG, 'Failed to remove target: ' + e); |
There was a problem hiding this comment.
Can we log the query associated with the target here?
There was a problem hiding this comment.
Unless we go out of our way, we don't have access to the query here. I logged the target ID instead. We should however already have this information from the Watch stream.
FWIW, I didn't add this logging to begin with since this is a debug log. The other logs were error logs in response to user behavior. They are always visible, and often without context.
…s-sdk into mrschmidt/watchretry
| await this.raiseWatchSnapshot(snapshotVersion); | ||
| } | ||
| } catch (e) { | ||
| logDebug(LOG_TAG, 'Failed to raise snapshot: ' + e); |
There was a problem hiding this comment.
The snapshot version probably isn't that useful, but the target ID would be. Anything to help associate this message with its context.
If the targetId isn't available going back to what you had before is probably better.
This PR takes the network offline if we fail to talk to IndexedDB when applying a query snapshot or rejection.
Also cleans up some Promise handling in RemoteStore.
Addresses #2755