-
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
Changes from all commits
d5b9b56
3d3246e
f8b87a7
e8ec6ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,8 @@ class SyncService { | |
|
||
this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL) | ||
|
||
this.pushError = 0 | ||
|
||
return this | ||
} | ||
|
||
|
@@ -88,8 +90,12 @@ class SyncService { | |
return this.#connection.session.guestName | ||
} | ||
|
||
get hasActiveConnection() { | ||
juliushaertl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return this.#connection && !this.#connection.isClosed | ||
} | ||
|
||
async open({ fileId, initialSession }) { | ||
if (this.#connection && !this.#connection.isClosed) { | ||
if (this.hasActiveConnection) { | ||
// We're already connected. | ||
return | ||
} | ||
|
@@ -166,6 +172,7 @@ class SyncService { | |
} | ||
return this.#connection.push(data) | ||
.then((response) => { | ||
this.pushError = 0 | ||
this.sending = false | ||
this.emit('sync', { | ||
steps: [], | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Treating push errors in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (!response || code === 'ECONNABORTED') { | ||
this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} }) | ||
} | ||
|
@@ -191,6 +199,8 @@ class SyncService { | |
this.emit('error', { type: ERROR_TYPE.PUSH_FAILURE, data: {} }) | ||
OC.Notification.showTemporary('Changes could not be sent yet') | ||
} | ||
} else { | ||
this.emit('error', { type: ERROR_TYPE.PUSH_FAILURE, data: {} }) | ||
} | ||
throw new Error('Failed to apply steps. Retry!', { cause: err }) | ||
}) | ||
|
@@ -301,7 +311,7 @@ class SyncService { | |
// Make sure to leave no pending requests behind. | ||
this.autosave.clear() | ||
this.backend?.disconnect() | ||
if (!this.#connection || this.#connection.isClosed) { | ||
if (!this.hasActiveConnection) { | ||
return | ||
} | ||
return this.#connection.close() | ||
|
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 whenSyncService
emits'sync'
, which in my understanding happen either if we pushed steps successfully (which meanspushError = 0
), or if we received steps from the server (which is not related topushError
).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.