Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix reconnecting websocket polyfill and error propagation during push #6200

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Fix reconnecting websocket polyfill and error propagation during push #6200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5b9b56
fix: Track push errors and do not reset error state if push fails but…
juliushaertl Aug 12, 2024
3d3246e
fix: Do not reset read only state if sync errors occurred
juliushaertl Aug 12, 2024
f8b87a7
fix: emit onerror for websocket and reopen connection on reconnect at…
juliushaertl Aug 12, 2024
e8ec6ea
fix: Emit error if push fails in other unhandled cases
juliushaertl Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/Editor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,14 +559,14 @@ export default {
this.document = document

this.syncError = null
const editable = !this.readOnly
const editable = !this.readOnly && !this.hasConnectionIssue
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
Copy link
Member

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).

Copy link
Member Author

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.

this.$nextTick(() => {
this.emit('sync-service:sync')
})
Expand Down
14 changes: 12 additions & 2 deletions src/services/SyncService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class SyncService {

this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL)

this.pushError = 0

return this
}

Expand All @@ -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
}
Expand Down Expand Up @@ -166,6 +172,7 @@ class SyncService {
}
return this.#connection.push(data)
.then((response) => {
this.pushError = 0
this.sending = false
this.emit('sync', {
steps: [],
Expand All @@ -175,6 +182,7 @@ class SyncService {
}).catch(err => {
const { response, code } = err
this.sending = false
this.pushError++
Copy link
Member

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.

Copy link
Member Author

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 (!response || code === 'ECONNABORTED') {
this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} })
}
Expand All @@ -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 })
})
Expand Down Expand Up @@ -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()
Expand Down
8 changes: 7 additions & 1 deletion src/services/WebSocketPolyfill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
this.#notifyPushBus?.on('notify_push', this.#onNotifyPush.bind(this))
this.url = url
logger.debug('WebSocketPolyfill#constructor', { url, fileId, initialSession })
if (syncService.hasActiveConnection) {
setTimeout(() => this.onopen?.(), 0)
}
this.#registerHandlers({
opened: ({ version, session }) => {
this.#version = version
Expand Down Expand Up @@ -88,7 +91,10 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
...queue.filter(s => !outbox.includes(s)),
)
return ret
}, err => logger.error(err))
}, err => {
logger.error(`Failed to push the queue with ${queue.length} steps to the server`, err)
this.onerror?.(err)
})
}

async close() {
Expand Down
Loading