From 4978b032946d3260ab204298166ee2f02389208a Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 24 Jun 2024 18:08:04 +0200 Subject: [PATCH 1/7] fix(sessionApi): only mark api as closed on success Signed-off-by: Max --- src/services/SessionApi.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/services/SessionApi.js b/src/services/SessionApi.js index b0296b19c8f..19edb076de8 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -178,9 +178,12 @@ export class Connection { } close() { - const promise = this.#post(this.#url(`session/${this.#document.id}/close`), this.#defaultParams) - this.closed = true - return promise + return this.#post( + this.#url(`session/${this.#document.id}/close`), + this.#defaultParams, + ).then(() => { + this.closed = true + }) } // To be used in Cypress tests only From b9fa132f343cc86acde7ed27d865f2df343231b7 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 24 Jun 2024 18:10:54 +0200 Subject: [PATCH 2/7] fix(reconnect): keep baseVersionEtag during reconnect `this.$syncService` is cleared during the `close` method. However we need the `baseVersionEtag` to ensure the editing session on the server is still in sync with our local ydoc. Signed-off-by: Max --- cypress/e2e/sync.spec.js | 37 +++++++++++++++++++++++-------------- src/components/Editor.vue | 5 +++-- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index f47fac08665..8b03438fa72 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -64,22 +64,11 @@ describe('Sync', () => { }) it('recovers from a short lost connection', () => { - let reconnect = false - cy.intercept('**/apps/text/session/*/*', (req) => { - if (reconnect) { - req.continue() - req.alias = 'alive' - } else { - req.destroy() - req.alias = 'dead' - } - }).as('sessionRequests') + cy.intercept('**/apps/text/session/*/*', req => req.destroy()).as('dead') cy.wait('@dead', { timeout: 30000 }) cy.get('#editor-container .document-status', { timeout: 30000 }) .should('contain', 'Document could not be loaded.') - .then(() => { - reconnect = true - }) + cy.intercept('**/apps/text/session/*/*', req => req.continue()).as('alive') cy.wait('@alive', { timeout: 30000 }) cy.intercept({ method: 'POST', url: '**/apps/text/session/*/sync' }) .as('syncAfterRecovery') @@ -97,6 +86,26 @@ describe('Sync', () => { .should('include', 'after the lost connection') }) + it('reconnects via button after a short lost connection', () => { + cy.intercept('**/apps/text/session/*/*', req => req.destroy()).as('dead') + cy.wait('@dead', { timeout: 30000 }) + cy.get('#editor-container .document-status', { timeout: 30000 }) + .should('contain', 'Document could not be loaded.') + cy.get('#editor-container .document-status') + .find('.button.primary').click() + cy.intercept('**/apps/text/session/*/*', req => { + if (req.url.endsWith('create')) { + req.alias = 'create' + } + req.continue() + }).as('alive') + cy.wait('@alive', { timeout: 30000 }) + cy.wait('@create', { timeout: 10000 }) + .its('request.body') + .should('have.property', 'baseVersionEtag') + .should('not.be.empty') + }) + it('recovers from a lost and closed connection', () => { let reconnect = false cy.intercept('**/apps/text/session/*/*', (req) => { @@ -128,7 +137,7 @@ describe('Sync', () => { .should('include', 'after the lost connection') }) - it('shows warning when document session got cleaned up', () => { + it('asks to reload page when document session got cleaned up', () => { cy.get('.save-status button') .click() cy.wait('@save') diff --git a/src/components/Editor.vue b/src/components/Editor.vue index 499a3bc67d4..02d64d9aadc 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -370,7 +370,7 @@ export default { guestName, shareToken: this.shareToken, filePath: this.relativePath, - baseVersionEtag: this.$syncService?.baseVersionEtag, + baseVersionEtag: this.$baseVersionEtag, forceRecreate: this.forceRecreate, serialize: this.isRichEditor ? (content) => createMarkdownSerializer(this.$editor.schema).serialize(content ?? this.$editor.state.doc) @@ -485,7 +485,7 @@ export default { }) }, - onLoaded({ documentSource, documentState }) { + onLoaded({ document, documentSource, documentState }) { if (documentState) { applyDocumentState(this.$ydoc, documentState, this.$providers[0]) // distribute additional state that may exist locally @@ -498,6 +498,7 @@ export default { this.setInitialYjsState(documentSource, { isRichEditor: this.isRichEditor }) } + this.$baseVersionEtag = document.baseVersionEtag this.hasConnectionIssue = false const language = extensionHighlight[this.fileExtension] || this.fileExtension; From 299c8e861e58b5b8c622cdafe5afe8bd6dcc44e5 Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 27 Jun 2024 16:49:54 +0200 Subject: [PATCH 3/7] refactor: hide connection in sync service Signed-off-by: Max --- src/components/Editor.vue | 13 ++--- src/components/Editor/GuestNameDialog.vue | 4 +- src/services/PollingBackend.js | 2 +- src/services/SessionApi.js | 4 ++ src/services/SyncService.js | 60 +++++++++++++---------- 5 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index 02d64d9aadc..e78c42759f1 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -661,14 +661,11 @@ export default { async close() { if (this.currentSession && this.$syncService) { - try { - await this.$syncService.close() - this.unlistenSyncServiceEvents() - this.currentSession = null - this.$syncService = null - } catch (e) { - // Ignore issues closing the session since those might happen due to network issues - } + await this.$syncService.close() + this.unlistenSyncServiceEvents() + this.$syncService = null + // disallow editing while still showing the content + this.readOnly = true } if (this.$editor) { try { diff --git a/src/components/Editor/GuestNameDialog.vue b/src/components/Editor/GuestNameDialog.vue index ccbc77488b0..76af1d4d46b 100644 --- a/src/components/Editor/GuestNameDialog.vue +++ b/src/components/Editor/GuestNameDialog.vue @@ -72,12 +72,12 @@ export default { }, }, beforeMount() { - this.guestName = this.$syncService.connection.session.guestName + this.guestName = this.$syncService.guestName this.updateBufferedGuestName() }, methods: { setGuestName() { - const previousGuestName = this.$syncService.connection.session.guestName + const previousGuestName = this.$syncService.guestName this.$syncService.updateSession(this.guestName).then(() => { localStorage.setItem('nick', this.guestName) this.updateBufferedGuestName() diff --git a/src/services/PollingBackend.js b/src/services/PollingBackend.js index 0d125f63366..5db2ffb3379 100644 --- a/src/services/PollingBackend.js +++ b/src/services/PollingBackend.js @@ -143,7 +143,7 @@ class PollingBackend { } const disconnect = Date.now() - COLLABORATOR_DISCONNECT_TIME const alive = sessions.filter((s) => s.lastContact * 1000 > disconnect) - if (this.#syncService.connection.state.document.readOnly) { + if (this.#syncService.isReadOnly) { this.maximumReadOnlyTimer() } else if (alive.length < 2) { this.maximumRefetchTimer() diff --git a/src/services/SessionApi.js b/src/services/SessionApi.js index 19edb076de8..4e596ba5adc 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -101,6 +101,10 @@ export class Connection { } } + get isClosed() { + return this.closed + } + get #defaultParams() { return { documentId: this.#document.id, diff --git a/src/services/SyncService.js b/src/services/SyncService.js index a68e55f90d2..3b7ad48a43f 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -67,6 +67,7 @@ const ERROR_TYPE = { class SyncService { #sendIntervalId + #connection constructor({ baseVersionEtag, serialize, getDocumentState, ...options }) { /** @type {import('mitt').Emitter} _bus */ @@ -75,7 +76,7 @@ class SyncService { this.serialize = serialize this.getDocumentState = getDocumentState this._api = new SessionApi(options) - this.connection = null + this.#connection = null this.sessions = [] @@ -94,6 +95,14 @@ class SyncService { return this } + get isReadOnly() { + return this.#connection.state.document.readOnly + } + + get guestName() { + return this.#connection.session.guestName + } + async open({ fileId, initialSession }) { const onChange = ({ sessions }) => { this.sessions = sessions @@ -105,21 +114,21 @@ class SyncService { : this._api.open({ fileId, baseVersionEtag: this.baseVersionEtag }) .catch(error => this._emitError(error)) - this.connection = await connect - if (!this.connection) { + this.#connection = await connect + if (!this.#connection) { this.off('change', onChange) // Error was already emitted in connect return } - this.backend = new PollingBackend(this, this.connection) - this.version = this.connection.docStateVersion - this.baseVersionEtag = this.connection.document.baseVersionEtag + this.backend = new PollingBackend(this, this.#connection) + this.version = this.#connection.docStateVersion + this.baseVersionEtag = this.#connection.document.baseVersionEtag this.emit('opened', { - ...this.connection.state, + ...this.#connection.state, version: this.version, }) this.emit('loaded', { - ...this.connection.state, + ...this.#connection.state, version: this.version, }) } @@ -141,10 +150,10 @@ class SyncService { } updateSession(guestName) { - if (!this.connection.isPublic) { + if (!this.#connection.isPublic) { return Promise.reject(new Error()) } - return this.connection.update(guestName) + return this.#connection.update(guestName) .catch((error) => { logger.error('Failed to update the session', { error }) return Promise.reject(error) @@ -158,7 +167,7 @@ class SyncService { } return new Promise((resolve, reject) => { this.#sendIntervalId = setInterval(() => { - if (this.connection && !this.sending) { + if (this.#connection && !this.sending) { this.sendStepsNow(getSendable).then(resolve).catch(reject) } }, 200) @@ -173,12 +182,12 @@ class SyncService { if (data.steps.length > 0) { this.emit('stateChange', { dirty: true }) } - return this.connection.push(data) + return this.#connection.push(data) .then((response) => { this.sending = false this.emit('sync', { steps: [], - document: this.connection.document, + document: this.#connection.document, version: this.version, }) }).catch(err => { @@ -235,7 +244,7 @@ class SyncService { this.emit('sync', { steps: newSteps, // TODO: do we actually need to dig into the connection here? - document: this.connection.document, + document: this.#connection.document, version: this.version, }) } @@ -257,7 +266,7 @@ class SyncService { async save({ force = false, manualSave = true } = {}) { logger.debug('[SyncService] saving', arguments[0]) try { - const response = await this.connection.save({ + const response = await this.#connection.save({ version: this.version, autosaveContent: this._getContent(), documentState: this.getDocumentState(), @@ -265,7 +274,7 @@ class SyncService { manualSave, }) this.emit('stateChange', { dirty: false }) - this.connection.document.lastSavedVersionTime = Date.now() / 1000 + this.#connection.document.lastSavedVersionTime = Date.now() / 1000 logger.debug('[SyncService] saved', response) const { document, sessions } = response.data this.emit('save', { document, sessions }) @@ -288,23 +297,22 @@ class SyncService { // Make sure to leave no pending requests behind. this.autosave.clear() this.backend?.disconnect() - return this._close() - } - - _close() { - if (this.connection === null) { - return Promise.resolve() + if (!this.#connection || this.#connection.isClosed) { + return } - this.backend.disconnect() - return this.connection.close() + return this.#connection.close() + // Log and ignore possible network issues. + .catch(e => { + logger.info('Failed to close connection.', { e }) + }) } uploadAttachment(file) { - return this.connection.uploadAttachment(file) + return this.#connection.uploadAttachment(file) } insertAttachmentFile(filePath) { - return this.connection.insertAttachmentFile(filePath) + return this.#connection.insertAttachmentFile(filePath) } on(event, callback) { From 3f58baba0bd4c4dcac3057546445a4df54419a7b Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 1 Jul 2024 09:46:53 +0200 Subject: [PATCH 4/7] test(cy): wait longer for initial sync to avoid timeouts Signed-off-by: Max --- cypress/e2e/sync.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index 8b03438fa72..cacdaccfe03 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -36,10 +36,10 @@ describe('Sync', () => { cy.intercept({ method: 'POST', url: '**/apps/text/session/*/sync' }).as('sync') cy.intercept({ method: 'POST', url: '**/apps/text/session/*/save' }).as('save') cy.openTestFile() - cy.wait('@sync') + cy.wait('@sync', { timeout: 10000 }) cy.getContent().find('h2').should('contain', 'Hello world') cy.getContent().type('{moveToEnd}* Saving the doc saves the doc state{enter}') - cy.wait('@sync') + cy.wait('@sync', { timeout: 10000 }) }) it('saves the actual file and document state', () => { From e993f5f4882bfe01a6edf06aedab2dff6c61d28f Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 1 Jul 2024 09:47:29 +0200 Subject: [PATCH 5/7] test(cy): also test failed reconnect attempt Signed-off-by: Max --- cypress/e2e/sync.spec.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index cacdaccfe03..715861bf65d 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -93,12 +93,15 @@ describe('Sync', () => { .should('contain', 'Document could not be loaded.') cy.get('#editor-container .document-status') .find('.button.primary').click() - cy.intercept('**/apps/text/session/*/*', req => { - if (req.url.endsWith('create')) { - req.alias = 'create' - } - req.continue() - }).as('alive') + cy.get('.toastify').should('contain', 'Connection failed.') + cy.get('.toastify', { timeout: 30000 }).should('not.exist') + cy.get('#editor-container .document-status', { timeout: 30000 }) + .should('contain', 'Document could not be loaded.') + // bring back the network connection + cy.intercept('**/apps/text/session/*/*', req => { req.continue() }).as('alive') + cy.intercept('**/apps/text/session/*/create').as('create') + cy.get('#editor-container .document-status') + .find('.button.primary').click() cy.wait('@alive', { timeout: 30000 }) cy.wait('@create', { timeout: 10000 }) .its('request.body') From 2c03655d9c08d51b6355ecf36fe35c8c0cf1da67 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 1 Jul 2024 09:49:41 +0200 Subject: [PATCH 6/7] fix(Editor): separate close and disconnect functions * `close` is for closing the editor. It tries to save the document and clean everything up. * `disconnect` is for cleaning up the current collaboration sessions. It will not save the document and asumes the editing will be resumed after a reconnect. Move `sendRemainingSteps` out to the sync service. Also make close in the websocket polyfill sync. Just clean up the polyfills state. Signed-off-by: Max --- src/components/Editor.vue | 25 ++++++++++++++----------- src/services/SyncService.js | 24 ++++++++++++++++++++++++ src/services/WebSocketPolyfill.js | 27 +-------------------------- 3 files changed, 39 insertions(+), 37 deletions(-) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index e78c42759f1..b716b6af69c 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -352,7 +352,7 @@ export default { const timeout = new Promise((resolve) => setTimeout(resolve, 2000)) await Promise.any([timeout, this.$syncService.save()]) } - this.$providers.forEach(p => p.destroy()) + this.close() }, methods: { ...mapActions('text', [ @@ -380,8 +380,6 @@ export default { this.listenSyncServiceEvents() - this.$providers.forEach(p => p?.destroy()) - this.$providers = [] const syncServiceProvider = createSyncServiceProvider({ ydoc: this.$ydoc, syncService: this.$syncService, @@ -429,7 +427,7 @@ export default { reconnect() { this.contentLoaded = false this.hasConnectionIssue = false - this.close().then(this.initSession) + this.disconnect().then(this.initSession) this.idle = false }, @@ -659,14 +657,19 @@ export default { await this.$syncService.save() }, + async disconnect() { + await this.$syncService.close() + this.unlistenSyncServiceEvents() + this.$providers.forEach(p => p?.destroy()) + this.$providers = [] + this.$syncService = null + // disallow editing while still showing the content + this.readOnly = true + }, + async close() { - if (this.currentSession && this.$syncService) { - await this.$syncService.close() - this.unlistenSyncServiceEvents() - this.$syncService = null - // disallow editing while still showing the content - this.readOnly = true - } + await this.$syncService.sendRemainingSteps(this.$queue) + await this.disconnect() if (this.$editor) { try { this.unlistenEditorEvents() diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 3b7ad48a43f..29ffe4a8d16 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -25,6 +25,7 @@ import debounce from 'debounce' import PollingBackend from './PollingBackend.js' import SessionApi, { Connection } from './SessionApi.js' +import { encodeArrayBuffer } from '../helpers/base64.ts' import { logger } from '../helpers/logger.js' /** @@ -293,6 +294,29 @@ class SyncService { return this.save({ manualSave: false }) } + async sendRemainingSteps(queue) { + if (queue.length === 0) { + return + } + let outbox = [] + const steps = queue.map(s => encodeArrayBuffer(s)) + .filter(s => s < 'AQ') + const awareness = queue.map(s => encodeArrayBuffer(s)) + .findLast(s => s > 'AQ') || '' + return this.sendStepsNow(() => { + const data = { steps, awareness, version: this.version } + outbox = [...queue] + logger.debug('sending final steps ', data) + return data + })?.then(() => { + // only keep the steps that were not send yet + queue.splice(0, + queue.length, + ...queue.filter(s => !outbox.includes(s)), + ) + }, err => logger.error(err)) + } + async close() { // Make sure to leave no pending requests behind. this.autosave.clear() diff --git a/src/services/WebSocketPolyfill.js b/src/services/WebSocketPolyfill.js index 9273027bf2e..b1214b9f8f3 100644 --- a/src/services/WebSocketPolyfill.js +++ b/src/services/WebSocketPolyfill.js @@ -114,37 +114,12 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio } async close() { - await this.#sendRemainingSteps() Object.entries(this.#handlers) .forEach(([key, value]) => syncService.off(key, value)) this.#handlers = [] - syncService.close().then(() => { - this.onclose() - }) + this.onclose() logger.debug('Websocket closed') } - #sendRemainingSteps() { - if (queue.length) { - let outbox = [] - return syncService.sendStepsNow(() => { - const data = { - steps: this.#steps, - awareness: this.#awareness, - version: this.#version, - } - outbox = [...queue] - logger.debug('sending final steps ', data) - return data - })?.then(() => { - // only keep the steps that were not send yet - queue.splice(0, - queue.length, - ...queue.filter(s => !outbox.includes(s)), - ) - }, err => logger.error(err)) - } - } - } } From a2b5ef228df1f459442758a486c042dd53390d6d Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 2 Jul 2024 09:11:13 +0200 Subject: [PATCH 7/7] refactor(yjs): move queue handling into helper Signed-off-by: Max Signed-off-by: Max --- src/helpers/yjs.js | 20 ++++++++++++++++++++ src/services/SyncService.js | 8 +++----- src/services/WebSocketPolyfill.js | 17 ++++------------- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/helpers/yjs.js b/src/helpers/yjs.js index a34331d2ebb..869626b693d 100644 --- a/src/helpers/yjs.js +++ b/src/helpers/yjs.js @@ -97,6 +97,26 @@ export function applyUpdateMessage(ydoc, updateMessage, origin = 'origin') { ) } +/** + * Get the steps for sending to the server + * + * @param {object[]} queue - queue for the outgoing steps + */ +export function getSteps(queue) { + return queue.map(s => encodeArrayBuffer(s)) + .filter(s => s < 'AQ') +} + +/** + * Encode the latest awareness message for sending + * + * @param {object[]} queue - queue for the outgoing steps + */ +export function getAwareness(queue) { + return queue.map(s => encodeArrayBuffer(s)) + .findLast(s => s > 'AQ') || '' +} + /** * Log y.js messages with their type and initiator call stack * diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 29ffe4a8d16..e05336d8a01 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -25,7 +25,7 @@ import debounce from 'debounce' import PollingBackend from './PollingBackend.js' import SessionApi, { Connection } from './SessionApi.js' -import { encodeArrayBuffer } from '../helpers/base64.ts' +import { getSteps, getAwareness } from '../helpers/yjs.js' import { logger } from '../helpers/logger.js' /** @@ -299,10 +299,8 @@ class SyncService { return } let outbox = [] - const steps = queue.map(s => encodeArrayBuffer(s)) - .filter(s => s < 'AQ') - const awareness = queue.map(s => encodeArrayBuffer(s)) - .findLast(s => s > 'AQ') || '' + const steps = getSteps(queue) + const awareness = getAwareness(queue) return this.sendStepsNow(() => { const data = { steps, awareness, version: this.version } outbox = [...queue] diff --git a/src/services/WebSocketPolyfill.js b/src/services/WebSocketPolyfill.js index b1214b9f8f3..ed5f743c198 100644 --- a/src/services/WebSocketPolyfill.js +++ b/src/services/WebSocketPolyfill.js @@ -21,7 +21,8 @@ */ import { logger } from '../helpers/logger.js' -import { encodeArrayBuffer, decodeArrayBuffer } from '../helpers/base64.js' +import { decodeArrayBuffer } from '../helpers/base64.js' +import { getSteps, getAwareness } from '../helpers/yjs.js' /** * @@ -86,8 +87,8 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio let outbox = [] return syncService.sendSteps(() => { const data = { - steps: this.#steps, - awareness: this.#awareness, + steps: getSteps(queue), + awareness: getAwareness(queue), version: this.#version, } outbox = [...queue] @@ -103,16 +104,6 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio }, err => logger.error(err)) } - get #steps() { - return queue.map(s => encodeArrayBuffer(s)) - .filter(s => s < 'AQ') - } - - get #awareness() { - return queue.map(s => encodeArrayBuffer(s)) - .findLast(s => s > 'AQ') || '' - } - async close() { Object.entries(this.#handlers) .forEach(([key, value]) => syncService.off(key, value))