Indicate syncState ERROR after many failed /syncs #410
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
when a /sync leads to an error, increase a counter and use the counter to decide which state to be in when starting keepAlives. If the count is above a certain threshold (arbitrary 3 chosen here), switch from RECONNECTING to ERROR to show that the server is facing difficulties that aren't affeting is ability to return on /versions.
dbkr
requested changes
Apr 4, 2017
src/sync.js
Outdated
| @@ -39,6 +39,13 @@ const DEBUG = true; | |||
| // to determine the max time we're willing to wait. | |||
| const BUFFER_PERIOD_MS = 80 * 1000; | |||
|
|
|||
| // Number of consecutive failed /sync requests | |||
| let failedSyncCount = 0; | |||
There was a problem hiding this comment.
This should be a member variable on SyncApi to match all the others (there could be multiple SyncApi objects).
src/sync.js
Outdated
| @@ -610,6 +617,9 @@ SyncApi.prototype._sync = function(syncOptions) { | |||
| client.store.save(); | |||
|
|
|||
| self._sync(syncOptions); | |||
|
|
|||
| // Reset after a successful sync | |||
| failedSyncCount = 0; | |||
There was a problem hiding this comment.
This wants to be within the _sync function in the done() (somewhere around client.store.setSyncToken(data.next_batch);). This doesn't do what you think because _sync doesn't block, ie. this will set failedSyncCount to 0 right after starting a sync request.
src/sync.js
Outdated
| self._updateSyncState("RECONNECTING"); | ||
| // Transition from RECONNECTING to ERROR after a given number of failed syncs | ||
| self._updateSyncState( | ||
| failedSyncCount > FAILED_SYNC_ERROR_THRESHOLD ? "ERROR" : "RECONNECTING", |
There was a problem hiding this comment.
Minor, but this probably wants to be >= rather than > so the ERROR state is triggered after FAILED_SYNC_ERROR_THRESHOLD failed syncs which is probably just a bit more intuitive than after n + 1 failed syncs which is what this does.
- failedSyncCount -> this._failedSyncCount - Reset at a point which is more obviously after a successful sync, and add comments to clarify the recursive syncage. - Sync state will now flip from RECONNECTING to ERROR after FAILED_SYNC_ERROR_THRESHOLD syncs
dbkr
approved these changes
Apr 5, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when a /sync leads to an error, increase a counter and use the counter to decide which state to be in when starting keepAlives. If the count is above a certain threshold (arbitrary 3 chosen here), switch from RECONNECTING to ERROR to show that the server is facing difficulties that aren't affeting is ability to return on /versions.
Attempts to fix element-hq/element-web#3336