-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix reconnecting websocket polyfill and error propagation during push #6200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to follow the logic by stepping through with a debugger and review, but have more questions than answers 😬
Maybe we can schedule a call later today or tomorrow to go through the changes together?
@@ -175,6 +182,7 @@ class SyncService { | |||
}).catch(err => { | |||
const { response, code } = err | |||
this.sending = false | |||
this.pushError++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treating push errors in hasConnectionIssues
means the editor turns readonly as soon as there a connection issue, right? I can imagine that usability will suffer in flaky networks (e.g. in trains) with this change. We could think about displaying a document status that warns about unpushed changes instead. But I'm also fine with merging this as is and seeing how it will perform in reality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After going through the code again I still feel this is more sane as a quick fix then skipping the commit. Without it we currently have even worse behaviour on flaky networks where the editor randomly switches between editable and non-editable depending if a sync request passes or not.
My proposal would be to get this in and follow up with a PR to improve the overall behaviour with error handling.
if (this.$editor.isEditable !== editable) { | ||
this.$editor.setEditable(editable) | ||
} | ||
}, | ||
|
||
onSync({ steps, document }) { | ||
this.hasConnectionIssue = false | ||
this.hasConnectionIssue = !this.$providers[0].wsconnected || this.$syncService.pushError > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm missing something obvious, but I don't understand why we should evaluate this.$syncService.pushError > 0
here. onSync()
is called when SyncService
emits 'sync'
, which in my understanding happen either if we pushed steps successfully (which means pushError = 0
), or if we received steps from the server (which is not related to pushError
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that push errors were always overwritten by any following sync request that passed. This change will make sure that a missing push is always still taken into account, so we display an error as long as no successful push happened.
e83d369
to
30663b0
Compare
@mejo- After testing the behaviour again with just the two commits as discussed yesterday i noticed that this is quite flaky that way:
I would feel more confident in merging this as it is and follow up to improve:
|
30663b0
to
f616406
Compare
… sync passes Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…tempts of y-websocket Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
f616406
to
e8ec6ea
Compare
Thanks for going through this again @juliushaertl. Your assessment sounds reasonable. And the list of follow-up todos is also a nice starting point! So let's get this merged as is. And probably we should also backport until |
/backport to stable29 |
/backport to stable28 |
Follow up started in #6202 |
From reading console logs of recent reports and checking back the access logs it seemed we had couple of cases where the editor was left in a state where it was editable but changes were no longer pushed to the server.
I could reproduce this for multiple scenarios:
Set the browser network tab to offline and wait until the y-websocket connection got closed after not receiving further messages
Manually return a failure status code like 401, 503, 403 on the push endpoint
fix: Track push errors and do not reset error state if push fails but sync passes
fix: Do not reset read only state if sync errors occurred
fix: emit onerror for websocket and reopen connection on reconnect attempts of y-websocket
fix: Emit error if push fails in other unhandled cases