From a7c03f230ed7994d6eabf10baa80e691d42ab18a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 12 Aug 2024 11:04:19 +0200 Subject: [PATCH 1/5] fix: Track push errors and do not reset error state if push fails but sync passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- src/components/Editor.vue | 2 +- src/services/SyncService.js | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index 3a5ac7490f6..fa1a688f676 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -578,7 +578,7 @@ export default { }, onSync({ steps, document }) { - this.hasConnectionIssue = false + this.hasConnectionIssue = !this.$providers[0].wsconnected || this.$syncService.pushError > 0 this.$nextTick(() => { this.emit('sync-service:sync') }) diff --git a/src/services/SyncService.js b/src/services/SyncService.js index f758a9afa55..1200cbf1aaa 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -92,6 +92,8 @@ class SyncService { this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL) + this.pushError = 0 + return this } @@ -181,6 +183,7 @@ class SyncService { } return this.#connection.push(data) .then((response) => { + this.pushError = 0 this.sending = false this.emit('sync', { steps: [], @@ -190,6 +193,7 @@ class SyncService { }).catch(err => { const { response, code } = err this.sending = false + this.pushError++ if (!response || code === 'ECONNABORTED') { this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} }) } From aaf09b7cae34c01b6f6c6e32c4198e8c4a1e1a01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 12 Aug 2024 11:05:52 +0200 Subject: [PATCH 2/5] fix: Do not reset read only state if sync errors occurred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- src/components/Editor.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index fa1a688f676..fb3497e1a06 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -571,7 +571,7 @@ 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) } From e450663f8142a33b7abb8220ccce0bc795a5db93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 12 Aug 2024 11:09:41 +0200 Subject: [PATCH 3/5] fix: emit onerror for websocket and reopen connection on reconnect attempts of y-websocket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- src/services/SyncService.js | 8 ++++++-- src/services/WebSocketPolyfill.js | 8 +++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 1200cbf1aaa..2a07c6341a9 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -105,8 +105,12 @@ class SyncService { return this.#connection.session.guestName } + get hasActiveConnection() { + return this.#connection && !this.#connection.isClosed + } + async open({ fileId, initialSession }) { - if (this.#connection && !this.#connection.isClosed) { + if (this.hasActiveConnection) { // We're already connected. return } @@ -320,7 +324,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() diff --git a/src/services/WebSocketPolyfill.js b/src/services/WebSocketPolyfill.js index ed5f743c198..8394b8dfaa3 100644 --- a/src/services/WebSocketPolyfill.js +++ b/src/services/WebSocketPolyfill.js @@ -47,6 +47,9 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio constructor(url) { 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 @@ -101,7 +104,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() { From 943b55d4913b0e86b71d813d1c401fcf8b3212c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 12 Aug 2024 11:10:23 +0200 Subject: [PATCH 4/5] fix: Emit error if push fails in other unhandled cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- src/services/SyncService.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 2a07c6341a9..3921b7115a3 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -214,6 +214,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 }) }) From 6bd8b244ac19e1b9e56c3d6ecc27db2e4dd2bb13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 16 Aug 2024 15:06:56 +0200 Subject: [PATCH 5/5] fix: Ensure WebsocketPolyfill always has the latest session state and version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- src/components/Editor.vue | 2 +- src/services/SyncService.js | 27 ++++++++++++-------- src/services/WebSocketPolyfill.js | 17 +++++++----- src/tests/services/WebsocketPolyfill.spec.js | 17 +++++++----- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index fb3497e1a06..05133005f9d 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -578,7 +578,7 @@ export default { }, onSync({ steps, document }) { - this.hasConnectionIssue = !this.$providers[0].wsconnected || this.$syncService.pushError > 0 + this.hasConnectionIssue = this.$syncService.backend.fetcher === 0 || !this.$providers[0].wsconnected || this.$syncService.pushError > 0 this.$nextTick(() => { this.emit('sync-service:sync') }) diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 3921b7115a3..577b932294d 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -109,10 +109,19 @@ class SyncService { return this.#connection && !this.#connection.isClosed } + get connectionState() { + if (!this.#connection || this.version === undefined) { + return null + } + return { + ...this.#connection.state, + version: this.version, + } + } + async open({ fileId, initialSession }) { if (this.hasActiveConnection) { - // We're already connected. - return + return this.connectionState } const connect = initialSession ? Promise.resolve(new Connection({ data: initialSession }, {})) @@ -121,19 +130,15 @@ class SyncService { this.#connection = await connect if (!this.#connection) { // Error was already emitted in connect - return + return null } this.backend = new PollingBackend(this, this.#connection) this.version = this.#connection.docStateVersion this.baseVersionEtag = this.#connection.document.baseVersionEtag - this.emit('opened', { - ...this.#connection.state, - version: this.version, - }) - this.emit('loaded', { - ...this.#connection.state, - version: this.version, - }) + this.emit('opened', this.connectionState) + this.emit('loaded', this.connectionState) + + return this.connectionState } startSync() { diff --git a/src/services/WebSocketPolyfill.js b/src/services/WebSocketPolyfill.js index 8394b8dfaa3..861204dfc9d 100644 --- a/src/services/WebSocketPolyfill.js +++ b/src/services/WebSocketPolyfill.js @@ -47,15 +47,11 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio constructor(url) { 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 logger.debug('opened ', { version, session }) + this.#version = version this.#session = session - this.onopen?.() }, loaded: ({ version, session, content }) => { logger.debug('loaded ', { version, session }) @@ -73,7 +69,16 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio } }, }) - syncService.open({ fileId, initialSession }) + + syncService.open({ fileId, initialSession }).then((data) => { + if (syncService.hasActiveConnection) { + const { version, session } = data + this.#version = version + this.#session = session + + this.onopen?.() + } + }) } #registerHandlers(handlers) { diff --git a/src/tests/services/WebsocketPolyfill.spec.js b/src/tests/services/WebsocketPolyfill.spec.js index 1536d6e6992..060bab71e67 100644 --- a/src/tests/services/WebsocketPolyfill.spec.js +++ b/src/tests/services/WebsocketPolyfill.spec.js @@ -3,21 +3,21 @@ import initWebSocketPolyfill from '../../services/WebSocketPolyfill.js' describe('Init function', () => { it('returns a websocket polyfill class', () => { - const syncService = { on: jest.fn(), open: jest.fn() } + const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) } const Polyfill = initWebSocketPolyfill(syncService) const websocket = new Polyfill('url') expect(websocket).toBeInstanceOf(Polyfill) }) it('registers handlers', () => { - const syncService = { on: jest.fn(), open: jest.fn() } + const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) } const Polyfill = initWebSocketPolyfill(syncService) const websocket = new Polyfill('url') expect(syncService.on).toHaveBeenCalled() }) it('opens sync service', () => { - const syncService = { on: jest.fn(), open: jest.fn() } + const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) } const fileId = 123 const initialSession = { } const Polyfill = initWebSocketPolyfill(syncService, fileId, initialSession) @@ -28,7 +28,7 @@ describe('Init function', () => { it('sends steps to sync service', async () => { const syncService = { on: jest.fn(), - open: jest.fn(), + open: jest.fn(() => Promise.resolve({ version: 123, session: {} })), sendSteps: async getData => getData(), } const queue = [ 'initial' ] @@ -46,9 +46,10 @@ describe('Init function', () => { }) it('handles early reject', async () => { + jest.spyOn(console, 'error').mockImplementation(() => {}) const syncService = { on: jest.fn(), - open: jest.fn(), + open: jest.fn(() => Promise.resolve({ version: 123, session: {} })), sendSteps: jest.fn().mockRejectedValue('error before reading steps in sync service'), } const queue = [ 'initial' ] @@ -64,9 +65,10 @@ describe('Init function', () => { }) it('handles reject after reading data', async () => { + jest.spyOn(console, 'error').mockImplementation(() => {}) const syncService = { on: jest.fn(), - open: jest.fn(), + open: jest.fn(() => Promise.resolve({ version: 123, session: {} })), sendSteps: jest.fn().mockImplementation( async getData => { getData() throw 'error when sending in sync service' @@ -85,9 +87,10 @@ describe('Init function', () => { }) it('queue survives a close', async () => { + jest.spyOn(console, 'error').mockImplementation(() => {}) const syncService = { on: jest.fn(), - open: jest.fn(), + open: jest.fn(() => Promise.resolve({ version: 123, session: {} })), sendSteps: jest.fn().mockImplementation( async getData => { getData() throw 'error when sending in sync service'