From 98cb9bf88ef60c5498ad455639ea7191527be0e8 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 3bf753d38cb..93388fe2930 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -161,9 +161,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 6093128143648a9f0997571d8f3e3f787a39bb38 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 5c58ffb2905..d50b9d65b30 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -47,22 +47,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') @@ -80,6 +69,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) => { @@ -111,7 +120,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 0217e4babc1..b3e9640f744 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -373,7 +373,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) @@ -487,7 +487,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 @@ -500,6 +500,7 @@ export default { this.setInitialYjsState(documentSource, { isRichEditor: this.isRichEditor }) } + this.$baseVersionEtag = document.baseVersionEtag this.hasConnectionIssue = false const language = extensionHighlight[this.fileExtension] || this.fileExtension; From a4ea9fcb5c780bb98b635cb9569231424cdb55c2 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 b3e9640f744..25c1d1e44a0 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -664,14 +664,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 cdc4f1ff735..61ae9e64133 100644 --- a/src/components/Editor/GuestNameDialog.vue +++ b/src/components/Editor/GuestNameDialog.vue @@ -55,12 +55,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 739da92feed..2c704ac1d25 100644 --- a/src/services/PollingBackend.js +++ b/src/services/PollingBackend.js @@ -126,7 +126,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 93388fe2930..11f6fb3fb5d 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -84,6 +84,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 349dd75f0d0..52029465ac4 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -52,6 +52,7 @@ const ERROR_TYPE = { class SyncService { #sendIntervalId + #connection constructor({ baseVersionEtag, serialize, getDocumentState, ...options }) { /** @type {import('mitt').Emitter} _bus */ @@ -60,7 +61,7 @@ class SyncService { this.serialize = serialize this.getDocumentState = getDocumentState this._api = new SessionApi(options) - this.connection = null + this.#connection = null this.stepClientIDs = [] @@ -76,6 +77,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 connect = initialSession @@ -83,20 +92,20 @@ 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) { // 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, }) } @@ -118,10 +127,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) @@ -135,7 +144,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) @@ -150,12 +159,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 => { @@ -210,7 +219,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, }) } @@ -232,7 +241,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(), @@ -240,7 +249,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 }) @@ -265,23 +274,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 eb9dcbb7831db949276e114e522552a0c0fbe7ad 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 d50b9d65b30..449366e6bf7 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -19,10 +19,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 97da812c56bc1a503ed9e8e9ca45a2e56799c67b 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 449366e6bf7..d2a4315685d 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -76,12 +76,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 e214f6f4c47e8f9a5fbe8d239e2d0396c4205e46 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 | 27 +++++++++++++++------------ src/services/SyncService.js | 24 ++++++++++++++++++++++++ src/services/WebSocketPolyfill.js | 27 +-------------------------- 3 files changed, 40 insertions(+), 38 deletions(-) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index 25c1d1e44a0..b15a15b0920 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -354,12 +354,12 @@ export default { } unsubscribe('text:image-node:add', this.onAddImageNode) unsubscribe('text:image-node:delete', this.onDeleteImageNode) + unsubscribe('text:translate-modal:show', this.showTranslateModal) if (this.dirty) { const timeout = new Promise((resolve) => setTimeout(resolve, 2000)) await Promise.any([timeout, this.$syncService.save()]) } - this.$providers.forEach(p => p.destroy()) - unsubscribe('text:translate-modal:show', this.showTranslateModal) + this.close() }, methods: { initSession() { @@ -383,8 +383,6 @@ export default { this.listenSyncServiceEvents() - this.$providers.forEach(p => p?.destroy()) - this.$providers = [] const syncServiceProvider = createSyncServiceProvider({ ydoc: this.$ydoc, syncService: this.$syncService, @@ -432,7 +430,7 @@ export default { reconnect() { this.contentLoaded = false this.hasConnectionIssue = false - this.close().then(this.initSession) + this.disconnect().then(this.initSession) this.idle = false }, @@ -662,14 +660,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 52029465ac4..db37c133f90 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -10,6 +10,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' /** @@ -270,6 +271,29 @@ class SyncService { }) } + 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 32eb0b96f68..aaad3df6a4b 100644 --- a/src/services/WebSocketPolyfill.js +++ b/src/services/WebSocketPolyfill.js @@ -97,37 +97,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 13c580c3d344517b0f75f285c4944beb71c98962 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 --- 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 c0d696a93d2..da202ca2d9c 100644 --- a/src/helpers/yjs.js +++ b/src/helpers/yjs.js @@ -80,6 +80,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 db37c133f90..648bc75d335 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -10,7 +10,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' /** @@ -276,10 +276,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 aaad3df6a4b..b06ba0d2631 100644 --- a/src/services/WebSocketPolyfill.js +++ b/src/services/WebSocketPolyfill.js @@ -4,7 +4,8 @@ */ import { logger } from '../helpers/logger.js' -import { encodeArrayBuffer, decodeArrayBuffer } from '../helpers/base64.ts' +import { decodeArrayBuffer } from '../helpers/base64.ts' +import { getSteps, getAwareness } from '../helpers/yjs.js' /** * @@ -69,8 +70,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] @@ -86,16 +87,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))