From a7bec4680c44d0376018f7e90b879619413693ed Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 13 Feb 2023 05:44:05 +0100 Subject: [PATCH 1/5] wip: lazy polling Signed-off-by: Max --- src/helpers/lazy.js | 127 ++++++++++++++++++++++++++++ src/tests/helpers/lazy.spec.js | 147 +++++++++++++++++++++++++++++++++ 2 files changed, 274 insertions(+) create mode 100644 src/helpers/lazy.js create mode 100644 src/tests/helpers/lazy.spec.js diff --git a/src/helpers/lazy.js b/src/helpers/lazy.js new file mode 100644 index 00000000000..d667a05aa8b --- /dev/null +++ b/src/helpers/lazy.js @@ -0,0 +1,127 @@ +/* + * @copyright Copyright (c) 2023 Max + * + * @author Max + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +/** + * + * @param {Function} callback to be triggered by the timer + */ +function lazyTimer(callback) { + const fn = lazy(callback) + fn.interval = setInterval(fn, 300) + return fn +} + +export { lazyTimer } + +/** + * Throttle a function so it only runs in intervals that double on every run. + * + * const fn = lazy(inner) + * fn() // call inner + * fn() // skip - so interval between inner() is 2x the interval between fn() + * fn() // call inner + * fn(); fn(); fn() // skip all - so interval is 4x the interval between fn() + * fn() // call inner + * + * fn.wakeUp() // will start from scratch. + * + * fn.sleep() // will skip `skipAtMost` steps between all runs until fn.wakeUp() is called. + * + * @param {Function} inner function to be called + * @param {object} options optional + * @param {number} options.skipAtMost maximum number of calls to skip, default: 15 + */ +export function lazy(inner, { skipAtMost = 15 } = {}) { + let count = 0 + const result = (...args) => { + count++ + if (runFor(count, skipAtMost)) { + return inner(...args) + } + } + result.wakeUp = () => { + count = 0 + } + result.sleep = () => { + const previousRun = runsBefore(count) + const previousCount = countAt(previousRun) + count = lastCountAfterDoubling(skipAtMost) + count - previousCount + } + return result +} + +/** + * @param {number} count time the function is being called + * @param {number} skipAtMost maximum number of calls to skip + */ +function runFor(count, skipAtMost) { + const nextRun = runsBefore(count) + 1 + const skips = skipsBefore(nextRun) + if (!skipAtMost || skips < skipAtMost) { + return count === countAt(nextRun) + } else { + const runEvery = skipAtMost + 1 + const result = (count - lastCountAfterDoubling(skipAtMost)) % runEvery === 0 + return result + } +} + +/** + * At what count does the inner function run for the nth time. + * + * @param {number} n time the inner function runs + * @return {number} + */ +function countAt(n) { + return 2 ** n - 1 +} + +/** + * How many runs happened before count. + * + * @param {number} count time the lazy function is being called + * @return {number} + */ +function runsBefore(count) { + return Math.floor(Math.log2(count)) +} + +/** + * How many calls of the lazy function are skipped before it runs the nth time. + * + * @param {number} n time the inner function runs + * @return {number} + */ +function skipsBefore(n) { + return (n === 1) ? 1 : countAt(n - 1) +} + +/** + * Count when the limit to doubling the intervals was reached. + * + * @param {number} skipAtMost upper limit for doubling + * @return {number} + */ +function lastCountAfterDoubling(skipAtMost) { + const lastRunToDoubleAfter = Math.floor(Math.log2(skipAtMost + 1)) + return countAt(lastRunToDoubleAfter + 1) +} diff --git a/src/tests/helpers/lazy.spec.js b/src/tests/helpers/lazy.spec.js new file mode 100644 index 00000000000..9a6a12c4e95 --- /dev/null +++ b/src/tests/helpers/lazy.spec.js @@ -0,0 +1,147 @@ +import { count } from 'lib0/indexeddb.js' +import { lazy, lazyTimer } from '../../helpers/lazy.js' + +describe('lazy timer', () => { + + test('runs on intervals that double', () => { + jest.useFakeTimers() + const callback = jest.fn(); + const timer = lazyTimer(callback); + expect(callback).not.toBeCalled(); + jest.advanceTimersByTime(400); + expect(callback).toHaveBeenCalledTimes(1); // 300 + jest.advanceTimersByTime(800); + expect(callback).toHaveBeenCalledTimes(2); // 900 (300 + 600) + jest.advanceTimersByTime(1600); + expect(callback).toHaveBeenCalledTimes(3); // 2100 (900 + 1200) + jest.advanceTimersByTime(3200); + expect(callback).toHaveBeenCalledTimes(4); // 4500 (2100 + 2400) + jest.advanceTimersByTime(6400); + expect(callback).toHaveBeenCalledTimes(5); // 9300 (4500 + 4800) + jest.useRealTimers() + }) + + test('stays at same interval once it reached maxInterval', () => { + jest.useFakeTimers() + const callback = jest.fn(); + const timer = lazyTimer(callback); + jest.advanceTimersByTime(10000); + expect(callback).toHaveBeenCalledTimes(5); // see above + jest.advanceTimersByTime(10000); + expect(callback).toHaveBeenCalledTimes(7); // 14100 (9300 + 4800) and 18900 (14100 + 4800) + jest.advanceTimersByTime(10000); + expect(callback).toHaveBeenCalledTimes(9); // roughly every 5 seconds + jest.useRealTimers() + }) + + test('starts from scratch after wakeUp', () => { + jest.useFakeTimers() + const callback = jest.fn(); + const timer = lazyTimer(callback); + jest.advanceTimersByTime(20000); + expect(callback).toHaveBeenCalledTimes(7); // see above + timer.wakeUp() + jest.advanceTimersByTime(400); + expect(callback).toHaveBeenCalledTimes(8); // 300 + jest.advanceTimersByTime(800); + expect(callback).toHaveBeenCalledTimes(9); // 900 (300 + 600) + jest.advanceTimersByTime(1600); + expect(callback).toHaveBeenCalledTimes(10); // 2100 (900 + 1200) + jest.useRealTimers() + }) + + test('goes to maxInterval when sleep is called', () => { + jest.useFakeTimers() + const callback = jest.fn(); + const timer = lazyTimer(callback); + jest.advanceTimersByTime(400); + expect(callback).toHaveBeenCalledTimes(1); // see above + timer.sleep() + jest.advanceTimersByTime(4000); + expect(callback).toHaveBeenCalledTimes(1); + jest.advanceTimersByTime(800); + expect(callback).toHaveBeenCalledTimes(2); + jest.advanceTimersByTime(4800); + expect(callback).toHaveBeenCalledTimes(3); + jest.useRealTimers() + }) + + test('allows to clear interval', () => { + jest.useFakeTimers() + const callback = jest.fn(); + const timer = lazyTimer(callback); + jest.advanceTimersByTime(400); + expect(callback).toHaveBeenCalledTimes(1); // see above + clearInterval(timer.interval) + jest.advanceTimersByTime(10000); + expect(callback).toHaveBeenCalledTimes(1); + jest.useRealTimers() + }) + + test('can stay awake by calling .wakeUp in callback', () => { + jest.useFakeTimers() + let timer + const callback = jest.fn(); + const energize = () => { + callback() + timer.wakeUp() + } + timer = lazyTimer(energize); + jest.advanceTimersByTime(3200); + expect(callback).toHaveBeenCalledTimes(10); + jest.useRealTimers() + }) + +}) + +describe('lazy function', () => { + + test('skips 1 than 3 than 7 than 15', () => { + const inner = jest.fn() + const fn = lazy(inner) + callNTimes(32, fn) + expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,15,31]) + }) + + test('starts again after being waken up', () => { + const inner = jest.fn() + const fn = lazy(inner) + callNTimes(3, fn) + fn.wakeUp() + callNTimes(10, fn) + expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,1,3,7]) + }) + + test('respects skipAtMost option', () => { + const inner = jest.fn() + const fn = lazy(inner, { skipAtMost: 3 }) + callNTimes(20, fn) + expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,11,15,19]) + }) + + test('skipAtMost defaults to 15', () => { + const inner = jest.fn() + const fn = lazy(inner) + callNTimes(64, fn) + expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,15,31,47,63]) + }) + + test('skips skipAtMost after sleep was called', () => { + const inner = jest.fn() + let count = 0 + const lazyFn = lazy(() => inner(count), { skipAtMost: 5 }) + const trigger = () => { + count++ + lazyFn() + } + callNTimes(4, trigger) + lazyFn.sleep() + callNTimes(20, trigger) + expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,9,15,21]) + }) + +}) + +function callNTimes(n, fn) { + for (let i = 1; i <= n; i++) { fn(i) } +} \ No newline at end of file From 8b662bd71137dc181671d012aece2fbdbac46d4f Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 14 Feb 2023 23:12:57 +0100 Subject: [PATCH 2/5] refactor(lazy): add interval variable to simplify Signed-off-by: Max --- src/helpers/lazy.js | 71 ++++------------------------------ src/tests/helpers/lazy.spec.js | 8 ++-- 2 files changed, 12 insertions(+), 67 deletions(-) diff --git a/src/helpers/lazy.js b/src/helpers/lazy.js index d667a05aa8b..e27a8fa4b1c 100644 --- a/src/helpers/lazy.js +++ b/src/helpers/lazy.js @@ -48,80 +48,25 @@ export { lazyTimer } * * @param {Function} inner function to be called * @param {object} options optional - * @param {number} options.skipAtMost maximum number of calls to skip, default: 15 + * @param {number} options.maxInterval maximum interval between two calls to inner */ -export function lazy(inner, { skipAtMost = 15 } = {}) { +export function lazy(inner, { maxInterval = 16 } = {}) { let count = 0 + let interval = 1 const result = (...args) => { count++ - if (runFor(count, skipAtMost)) { + if (count === interval) { + count = 0 + interval = Math.min(interval * 2, maxInterval) return inner(...args) } } result.wakeUp = () => { count = 0 + interval = 1 } result.sleep = () => { - const previousRun = runsBefore(count) - const previousCount = countAt(previousRun) - count = lastCountAfterDoubling(skipAtMost) + count - previousCount + interval = maxInterval } return result } - -/** - * @param {number} count time the function is being called - * @param {number} skipAtMost maximum number of calls to skip - */ -function runFor(count, skipAtMost) { - const nextRun = runsBefore(count) + 1 - const skips = skipsBefore(nextRun) - if (!skipAtMost || skips < skipAtMost) { - return count === countAt(nextRun) - } else { - const runEvery = skipAtMost + 1 - const result = (count - lastCountAfterDoubling(skipAtMost)) % runEvery === 0 - return result - } -} - -/** - * At what count does the inner function run for the nth time. - * - * @param {number} n time the inner function runs - * @return {number} - */ -function countAt(n) { - return 2 ** n - 1 -} - -/** - * How many runs happened before count. - * - * @param {number} count time the lazy function is being called - * @return {number} - */ -function runsBefore(count) { - return Math.floor(Math.log2(count)) -} - -/** - * How many calls of the lazy function are skipped before it runs the nth time. - * - * @param {number} n time the inner function runs - * @return {number} - */ -function skipsBefore(n) { - return (n === 1) ? 1 : countAt(n - 1) -} - -/** - * Count when the limit to doubling the intervals was reached. - * - * @param {number} skipAtMost upper limit for doubling - * @return {number} - */ -function lastCountAfterDoubling(skipAtMost) { - const lastRunToDoubleAfter = Math.floor(Math.log2(skipAtMost + 1)) - return countAt(lastRunToDoubleAfter + 1) -} diff --git a/src/tests/helpers/lazy.spec.js b/src/tests/helpers/lazy.spec.js index 9a6a12c4e95..bb170750c3d 100644 --- a/src/tests/helpers/lazy.spec.js +++ b/src/tests/helpers/lazy.spec.js @@ -114,22 +114,22 @@ describe('lazy function', () => { test('respects skipAtMost option', () => { const inner = jest.fn() - const fn = lazy(inner, { skipAtMost: 3 }) + const fn = lazy(inner, { maxInterval: 4 }) callNTimes(20, fn) expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,11,15,19]) }) - test('skipAtMost defaults to 15', () => { + test('maxInterval defaults to 16', () => { const inner = jest.fn() const fn = lazy(inner) callNTimes(64, fn) expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,15,31,47,63]) }) - test('skips skipAtMost after sleep was called', () => { + test('Uses maxInterval after sleep was called', () => { const inner = jest.fn() let count = 0 - const lazyFn = lazy(() => inner(count), { skipAtMost: 5 }) + const lazyFn = lazy(() => inner(count), { maxInterval: 6 }) const trigger = () => { count++ lazyFn() From f16693d89d169f131fc5f02391d8f00c63acd618 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 14 Feb 2023 23:48:17 +0100 Subject: [PATCH 3/5] enh(lazyTimer): options minInterval and maxDelay Signed-off-by: Max --- src/helpers/lazy.js | 25 ++++++++++++++++++------- src/tests/helpers/lazy.spec.js | 24 ++++++++++++++++++------ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/helpers/lazy.js b/src/helpers/lazy.js index e27a8fa4b1c..2fc136ae3f4 100644 --- a/src/helpers/lazy.js +++ b/src/helpers/lazy.js @@ -21,12 +21,23 @@ */ /** + * Call callback in intervals that double + * + * Returns a handle to modify the interval: + * - handle.wakeUp() resets it to the minimum. + * - handle.sleep() uses maximum interval right away. + * - handle.interval can be used to clear the interval: + * `clearInterval(handle.interval)` * * @param {Function} callback to be triggered by the timer + * @param {object} options optional + * @param {number} options.minInterval minimum interval between two calls + * @param {number} options.maxDelay maximum factor to multiply the interval by + * @return {Function} handle to modify behavior */ -function lazyTimer(callback) { - const fn = lazy(callback) - fn.interval = setInterval(fn, 300) +function lazyTimer(callback, { minInterval = 300, maxDelay = 16 } = {}) { + const fn = lazy(callback, { maxDelay }) + fn.interval = setInterval(fn, minInterval) return fn } @@ -48,16 +59,16 @@ export { lazyTimer } * * @param {Function} inner function to be called * @param {object} options optional - * @param {number} options.maxInterval maximum interval between two calls to inner + * @param {number} options.maxDelay maximum interval between two calls to inner */ -export function lazy(inner, { maxInterval = 16 } = {}) { +export function lazy(inner, { maxDelay = 16 } = {}) { let count = 0 let interval = 1 const result = (...args) => { count++ if (count === interval) { count = 0 - interval = Math.min(interval * 2, maxInterval) + interval = Math.min(interval * 2, maxDelay) return inner(...args) } } @@ -66,7 +77,7 @@ export function lazy(inner, { maxInterval = 16 } = {}) { interval = 1 } result.sleep = () => { - interval = maxInterval + interval = maxDelay } return result } diff --git a/src/tests/helpers/lazy.spec.js b/src/tests/helpers/lazy.spec.js index bb170750c3d..72c8a36a7fb 100644 --- a/src/tests/helpers/lazy.spec.js +++ b/src/tests/helpers/lazy.spec.js @@ -21,7 +21,19 @@ describe('lazy timer', () => { jest.useRealTimers() }) - test('stays at same interval once it reached maxInterval', () => { + test('allows configuring min and max intervals', () => { + jest.useFakeTimers() + const callback = jest.fn(); + const timer = lazyTimer(callback, { minInterval: 10, maxDelay: 2 }); + expect(callback).not.toBeCalled(); + jest.advanceTimersByTime(40); + expect(callback).toHaveBeenCalledTimes(2); // 10, 30 + jest.advanceTimersByTime(40); + expect(callback).toHaveBeenCalledTimes(4); // 50, 70 + jest.useRealTimers() + }) + + test('stays at same interval once it reached maxDelay', () => { jest.useFakeTimers() const callback = jest.fn(); const timer = lazyTimer(callback); @@ -50,7 +62,7 @@ describe('lazy timer', () => { jest.useRealTimers() }) - test('goes to maxInterval when sleep is called', () => { + test('goes to maxDelay when sleep is called', () => { jest.useFakeTimers() const callback = jest.fn(); const timer = lazyTimer(callback); @@ -114,22 +126,22 @@ describe('lazy function', () => { test('respects skipAtMost option', () => { const inner = jest.fn() - const fn = lazy(inner, { maxInterval: 4 }) + const fn = lazy(inner, { maxDelay: 4 }) callNTimes(20, fn) expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,11,15,19]) }) - test('maxInterval defaults to 16', () => { + test('maxDelay defaults to 16', () => { const inner = jest.fn() const fn = lazy(inner) callNTimes(64, fn) expect(inner.mock.calls.map(call => call[0])).toEqual([1,3,7,15,31,47,63]) }) - test('Uses maxInterval after sleep was called', () => { + test('Uses maxDelay after sleep was called', () => { const inner = jest.fn() let count = 0 - const lazyFn = lazy(() => inner(count), { maxInterval: 6 }) + const lazyFn = lazy(() => inner(count), { maxDelay: 6 }) const trigger = () => { count++ lazyFn() From 4f0ec373b59f44d4d318568a2140319d1e181f1b Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 16 Feb 2023 20:13:21 +0100 Subject: [PATCH 4/5] cleanup(SyncService): this.sessions and this.steps Both were kept up to date but never used as far as i can tell. Signed-off-by: Max --- src/services/SyncService.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 136756c543a..2fae170f46a 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -71,9 +71,6 @@ class SyncService { this._api = new SessionApi(options) this.connection = null - this.sessions = [] - - this.steps = [] this.stepClientIDs = [] this.lastStepPush = Date.now() @@ -87,9 +84,6 @@ class SyncService { } async open({ fileId, initialSession }) { - this.on('change', ({ sessions }) => { - this.sessions = sessions - }) // TODO: Only continue if a connection was made this.connection = initialSession @@ -178,7 +172,6 @@ class SyncService { return { step: s.lastAwarenessMessage, clientId: s.clientId } }) const newSteps = awareness - this.steps = [...this.steps, ...awareness.map(s => s.step)] for (let i = 0; i < steps.length; i++) { const singleSteps = steps[i].data if (this.version < steps[i].version) { @@ -190,7 +183,6 @@ class SyncService { continue } singleSteps.forEach(step => { - this.steps.push(step) newSteps.push({ step, clientID: steps[i].sessionId, From 742edb32123f6aff45a54c07e57425788377f492 Mon Sep 17 00:00:00 2001 From: Max Date: Fri, 17 Feb 2023 15:57:40 +0100 Subject: [PATCH 5/5] enh: save api request separate from sync Save will only save the doc and doc state. Sync will only fetch steps from the server. Signed-off-by: Max --- appinfo/routes.php | 2 + cypress/e2e/SessionApi.spec.js | 12 +++--- cypress/e2e/sync.spec.js | 16 ++++--- cypress/support/sessions.js | 5 +++ lib/Controller/PublicSessionController.php | 16 +++++-- lib/Controller/SessionController.php | 17 ++++++-- lib/Service/ApiService.php | 36 ++++++++++------ lib/Service/DocumentService.php | 22 ++++------ src/components/Editor.vue | 31 ++++++++------ src/services/PollingBackend.js | 26 +++++------ src/services/SessionApi.js | 9 +++- src/services/SyncService.js | 50 +++++++++++----------- 12 files changed, 143 insertions(+), 99 deletions(-) diff --git a/appinfo/routes.php b/appinfo/routes.php index a9b511c51c8..88456136aab 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -35,6 +35,7 @@ ['name' => 'Attachment#getMediaFileMetadata', 'url' => '/mediaMetadata', 'verb' => 'GET'], ['name' => 'Session#create', 'url' => '/session/create', 'verb' => 'PUT'], + ['name' => 'Session#save', 'url' => '/session/save', 'verb' => 'POST'], ['name' => 'Session#sync', 'url' => '/session/sync', 'verb' => 'POST'], ['name' => 'Session#push', 'url' => '/session/push', 'verb' => 'POST'], ['name' => 'Session#close', 'url' => '/session/close', 'verb' => 'POST'], @@ -42,6 +43,7 @@ ['name' => 'PublicSession#create', 'url' => '/public/session/create', 'verb' => 'PUT'], ['name' => 'PublicSession#updateSession', 'url' => '/public/session', 'verb' => 'POST'], + ['name' => 'PublicSession#save', 'url' => '/public/session/save', 'verb' => 'POST'], ['name' => 'PublicSession#sync', 'url' => '/public/session/sync', 'verb' => 'POST'], ['name' => 'PublicSession#push', 'url' => '/public/session/push', 'verb' => 'POST'], diff --git a/cypress/e2e/SessionApi.spec.js b/cypress/e2e/SessionApi.spec.js index 9287360df77..02c30eef89b 100644 --- a/cypress/e2e/SessionApi.spec.js +++ b/cypress/e2e/SessionApi.spec.js @@ -130,12 +130,14 @@ describe('The session Api', function() { }) cy.pushSteps({ connection, steps: [messages.query], version }) .then(response => { + // no steps inserted cy.wrap(response) .its('version') .should('eql', 0) + // but returns all 3 other samples cy.wrap(response) .its('steps.length') - .should('eql', 0) + .should('eql', 3) }) }) }) @@ -171,7 +173,7 @@ describe('The session Api', function() { cy.pushSteps({ connection, steps: [messages.update], version }) .its('version') .should('eql', 1) - cy.syncSteps(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true }) + cy.saveDoc(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true }) cy.downloadFile(filePath) .its('data') .should('eql', '# Heading 1') @@ -182,7 +184,7 @@ describe('The session Api', function() { cy.pushSteps({ connection, steps: [messages.update], version }) .its('version') .should('eql', 1) - cy.syncSteps(connection, { + cy.saveDoc(connection, { version: 1, autosaveContent: '# Heading 1', documentState, @@ -245,7 +247,7 @@ describe('The session Api', function() { cy.pushSteps({ connection, steps: [messages.update], version }) .its('version') .should('eql', 1) - cy.syncSteps(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true }) + cy.saveDoc(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true }) cy.login(user) cy.prepareSessionApi() cy.downloadFile('saves.md') @@ -258,7 +260,7 @@ describe('The session Api', function() { cy.pushSteps({ connection, steps: [messages.update], version }) .its('version') .should('eql', 1) - cy.syncSteps(connection, { + cy.saveDoc(connection, { version: 1, autosaveContent: '# Heading 1', documentState, diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index 6a206eb3abb..88b80375009 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -37,25 +37,29 @@ describe('yjs document state', () => { cy.openTestFile() cy.getContent().find('h2').should('contain', 'Hello world') cy.wait('@sync', { timeout: 7000 }) - cy.getContent().type('* Saving the doc saves the doc state{enter}') - cy.wait('@sync', { timeout: 7000 }) }) - it('saves the actual file and document state', () => { + it('saves the actual file', () => { + cy.getContent().type('* Saving the doc saves the file{enter}') + cy.wait('@sync', { timeout: 7000 }) + cy.intercept({ method: 'POST', url: '**/apps/text/session/save' }).as('save') cy.getContent().type('{ctrl+s}') - cy.wait('@sync').its('request.body') + cy.wait('@save').its('request.body') .should('have.property', 'autosaveContent') .should('not.be.empty') cy.closeFile() cy.testName() .then(name => cy.downloadFile(`/${name}.md`)) .its('data') - .should('include', 'saves the doc state') + .should('include', 'saves the file') }) it('passes the doc content from one session to the next', () => { + cy.getContent().type('* Saving the doc saves the doc state{enter}') + cy.wait('@sync', { timeout: 7000 }) + cy.intercept({ method: 'POST', url: '**/apps/text/session/save' }).as('save') cy.getContent().type('{ctrl+s}') - cy.wait('@sync').its('request.body') + cy.wait('@save').its('request.body') .should('have.property', 'documentState') .should('not.be.empty') cy.closeFile() diff --git a/cypress/support/sessions.js b/cypress/support/sessions.js index 3774e384d1d..528670e3e20 100644 --- a/cypress/support/sessions.js +++ b/cypress/support/sessions.js @@ -51,3 +51,8 @@ Cypress.Commands.add('syncSteps', (connection, options = { version: 0 }) => { return connection.sync(options) .then(response => response.data) }) + +Cypress.Commands.add('saveDoc', (connection, options = { }) => { + return connection.save(options) + .then(response => response.data) +}) diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index 6574edc2e3d..168741cd9ec 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -82,16 +82,24 @@ public function close(int $documentId, int $sessionId, string $sessionToken): Da * @NoAdminRequired * @PublicPage */ - public function push(int $documentId, int $sessionId, string $sessionToken, int $version, array $steps, string $awareness, string $token): DataResponse { - return $this->apiService->push($documentId, $sessionId, $sessionToken, $version, $steps, $awareness, $token); + public function push(int $documentId, int $sessionId, string $sessionToken, array $steps, string $awareness, string $token): DataResponse { + return $this->apiService->push($documentId, $sessionId, $sessionToken, $steps, $awareness, $token); } /** * @NoAdminRequired * @PublicPage */ - public function sync(string $token, int $documentId, int $sessionId, string $sessionToken, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse { - return $this->apiService->sync($documentId, $sessionId, $sessionToken, $version, $autosaveContent, $documentState, $force, $manualSave, $token); + public function sync(string $token, int $documentId, int $sessionId, string $sessionToken, int $version = 0): DataResponse { + return $this->apiService->sync($documentId, $sessionId, $sessionToken, $version); + } + + /** + * @NoAdminRequired + * @PublicPage + */ + public function save(string $token, int $documentId, int $sessionId, string $sessionToken, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse { + return $this->apiService->save($documentId, $sessionId, $sessionToken, $version, $autosaveContent, $documentState, $force, $manualSave, $token); } /** diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 63f5d8f4c9c..a40ce0f6728 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -69,18 +69,27 @@ public function close(int $documentId, int $sessionId, string $sessionToken): Da * @NoAdminRequired * @PublicPage */ - public function push(int $documentId, int $sessionId, string $sessionToken, int $version, array $steps, string $awareness): DataResponse { + public function push(int $documentId, int $sessionId, string $sessionToken, array $steps, string $awareness): DataResponse { $this->loginSessionUser($documentId, $sessionId, $sessionToken); - return $this->apiService->push($documentId, $sessionId, $sessionToken, $version, $steps, $awareness); + return $this->apiService->push($documentId, $sessionId, $sessionToken, $steps, $awareness); } /** * @NoAdminRequired * @PublicPage */ - public function sync(int $documentId, int $sessionId, string $sessionToken, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse { + public function sync(int $documentId, int $sessionId, string $sessionToken, int $version = 0): DataResponse { $this->loginSessionUser($documentId, $sessionId, $sessionToken); - return $this->apiService->sync($documentId, $sessionId, $sessionToken, $version, $autosaveContent, $documentState, $force, $manualSave); + return $this->apiService->sync($documentId, $sessionId, $sessionToken, $version); + } + + /** + * @NoAdminRequired + * @PublicPage + */ + public function save(int $documentId, int $sessionId, string $sessionToken, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse { + $this->loginSessionUser($documentId, $sessionId, $sessionToken); + return $this->apiService->save($documentId, $sessionId, $sessionToken, $version, $autosaveContent, $documentState, $force, $manualSave); } /** diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 176267b10ab..fb2aabaa09a 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -189,7 +189,7 @@ public function close($documentId, $sessionId, $sessionToken): DataResponse { * @throws NotFoundException * @throws DoesNotExistException */ - public function push($documentId, $sessionId, $sessionToken, $version, $steps, $awareness, $token = null): DataResponse { + public function push($documentId, $sessionId, $sessionToken, $steps, $awareness, $token = null): DataResponse { if (!$this->sessionService->isValidSession($documentId, $sessionId, $sessionToken)) { return new DataResponse([], 403); } @@ -201,7 +201,7 @@ public function push($documentId, $sessionId, $sessionToken, $version, $steps, $ $file = $this->documentService->getFileForSession($session, $token); if (!$this->documentService->isReadOnly($file, $token)) { try { - $result = $this->documentService->addStep($documentId, $sessionId, $steps, $version); + $result = $this->documentService->addStep($documentId, $sessionId, $steps); } catch (InvalidArgumentException $e) { return new DataResponse($e->getMessage(), 422); } @@ -210,18 +210,25 @@ public function push($documentId, $sessionId, $sessionToken, $version, $steps, $ return new DataResponse([], 403); } - public function sync($documentId, $sessionId, $sessionToken, $version = 0, $autosaveContent = null, $documentState = null, bool $force = false, bool $manualSave = false, $token = null): DataResponse { + public function sync($documentId, $sessionId, $sessionToken, $version = 0): DataResponse { if (!$this->sessionService->isValidSession($documentId, $sessionId, $sessionToken)) { return new DataResponse([], 403); } - try { - $result = [ - 'steps' => $this->documentService->getSteps($documentId, $version), - 'sessions' => $this->sessionService->getAllSessions($documentId), - 'document' => $this->documentService->get($documentId) - ]; + $result = [ + 'steps' => $this->documentService->getSteps($documentId, $version), + 'sessions' => $this->sessionService->getAllSessions($documentId), + ]; + return new DataResponse($result, 200); + } + + public function save($documentId, $sessionId, $sessionToken, $version = 0, $autosaveContent = null, $documentState = null, bool $force = false, bool $manualSave = false, $token = null): DataResponse { + $file = null; + if (!$this->sessionService->isValidSession($documentId, $sessionId, $sessionToken)) { + return new DataResponse([], 403); + } + try { $session = $this->sessionService->getSession($documentId, $sessionId, $sessionToken); $file = $this->documentService->getFileForSession($session, $token); } catch (NotFoundException $e) { @@ -237,10 +244,12 @@ public function sync($documentId, $sessionId, $sessionToken, $version = 0, $auto } try { - $result['document'] = $this->documentService->autosave($file, $documentId, $version, $autosaveContent, $documentState, $force, $manualSave, $token, $this->request->getParam('filePath')); + $document = $this->documentService->autosave($file, $documentId, $version, $autosaveContent, $documentState, $force, $manualSave, $token, $this->request->getParam('filePath')); } catch (DocumentSaveConflictException $e) { try { - $result['outsideChange'] = $file->getContent(); + return new DataResponse([ + 'outsideChange' => $file->getContent() + ], 409); } catch (LockedException $e) { // Ignore locked exception since it might happen due to an autosave action happening at the same time } @@ -252,8 +261,9 @@ public function sync($documentId, $sessionId, $sessionToken, $version = 0, $auto 'message' => 'Failed to autosave document' ], 500); } - - return new DataResponse($result, isset($result['outsideChange']) ? 409 : 200); + return new DataResponse([ + 'document' => $document + ]); } /** diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index a6f5d612c97..f6279efd20a 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -171,16 +171,15 @@ public function get($documentId) { * @param $documentId * @param $sessionId * @param $steps - * @param $version * @return array * @throws DoesNotExistException * @throws InvalidArgumentException */ - public function addStep($documentId, $sessionId, $steps, $version): array { + public function addStep($documentId, $sessionId, $steps): array { $stepsToInsert = []; $querySteps = []; $getStepsSinceVersion = null; - $newVersion = $version; + $newVersion = 0; foreach ($steps as $step) { // Steps are base64 encoded messages of the yjs protocols // https://github.com/yjs/y-protocols @@ -193,17 +192,13 @@ public function addStep($documentId, $sessionId, $steps, $version): array { } } if (sizeof($stepsToInsert) > 0) { - $newVersion = $this->insertSteps($documentId, $sessionId, $stepsToInsert, $version); + $newVersion = $this->insertSteps($documentId, $sessionId, $stepsToInsert); } // If there were any queries in the steps send the entire history - $getStepsSinceVersion = count($querySteps) > 0 ? 0 : $version; - $allSteps = $this->getSteps($documentId, $getStepsSinceVersion); - $stepsToReturn = []; - foreach ($allSteps as $step) { - if ($step < "AAQ") { - array_push($stepsToReturn, $step); - } - } + // TODO: make use of the saved document state here - otherwise it migth be too big. + $stepsToReturn = count($querySteps) === 0 + ? [] + : $this->getSteps($documentId, 0); return [ 'steps' => $stepsToReturn, 'version' => $newVersion @@ -214,12 +209,11 @@ public function addStep($documentId, $sessionId, $steps, $version): array { * @param $documentId * @param $sessionId * @param $steps - * @param $version * @return int * @throws DoesNotExistException * @throws InvalidArgumentException */ - private function insertSteps($documentId, $sessionId, $steps, $version): int { + private function insertSteps($documentId, $sessionId, $steps): int { $document = null; $stepsVersion = null; try { diff --git a/src/components/Editor.vue b/src/components/Editor.vue index c2112e82bbf..7b728e83e12 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -241,6 +241,7 @@ export default { forceRecreate: false, menubarLoaded: false, draggedOver: false, + version: 0, contentWrapper: null, } @@ -421,7 +422,7 @@ export default { }, resolveUseThisVersion() { - this.$syncService.forceSave() + this.$syncService.forceSave(this.version) this.$editor.setOptions({ editable: !this.readOnly }) }, @@ -429,7 +430,7 @@ export default { const markdownItHtml = markdownit.render(this.syncError.data.outsideChange) this.$editor.setOptions({ editable: !this.readOnly }) this.$editor.commands.setContent(markdownItHtml) - this.$syncService.forceSave() + this.$syncService.forceSave(this.version) }, reconnect() { @@ -527,7 +528,7 @@ export default { }), Keymap.configure({ 'Mod-s': () => { - this.$syncService.save() + this.$syncService.save(this.version) return true }, }), @@ -544,16 +545,22 @@ export default { }, + // TODO: split up in two handlers: + // - one that gets the session from PollingBackend + // - one that gets the document from SyncService onChange({ document, sessions }) { - if (this.document.baseVersionEtag !== '' && document.baseVersionEtag !== this.document.baseVersionEtag) { - this.resolveUseServerVersion() - return + if (document) { + if (this.document.baseVersionEtag !== '' && document.baseVersionEtag !== this.document.baseVersionEtag) { + this.resolveUseServerVersion() + return + } + this.document = document + this.$editor.setOptions({ editable: !this.readOnly }) + } + if (sessions) { + this.updateSessions.bind(this)(sessions) + this.syncError = null } - this.updateSessions.bind(this)(sessions) - this.document = document - - this.syncError = null - this.$editor.setOptions({ editable: !this.readOnly }) }, onSync({ steps, document }) { @@ -619,7 +626,7 @@ export default { ) { this.dirty = state.dirty if (this.dirty) { - this.$syncService.autosave() + this.$syncService.autosave(this.version) } } } diff --git a/src/services/PollingBackend.js b/src/services/PollingBackend.js index ff204ba2724..96686a108f9 100644 --- a/src/services/PollingBackend.js +++ b/src/services/PollingBackend.js @@ -63,6 +63,7 @@ const COLLABORATOR_DISCONNECT_TIME = FETCH_INTERVAL_INVISIBLE * 1.5 class PollingBackend { + version /** @type {SyncService} */ #syncService /** @type {Connection} */ @@ -80,6 +81,7 @@ class PollingBackend { this.#fetchInterval = FETCH_INTERVAL this.#fetchRetryCounter = 0 this.#lastPoll = 0 + this.version = 0 } connect() { @@ -114,9 +116,9 @@ class PollingBackend { this.#pollActive = true try { - logger.debug('[PollingBackend] Fetching steps', this.#syncService.version) + logger.debug('[PollingBackend] Fetching steps', this.version) const response = await this.#connection.sync({ - version: this.#syncService.version, + version: this.version, force: false, manualSave: false, }) @@ -130,11 +132,16 @@ class PollingBackend { } _handleResponse({ data }) { - const { document, sessions } = data + const { sessions } = data this.#fetchRetryCounter = 0 - this.#syncService.emit('change', { document, sessions }) + const version = Math.max( + this.version, + ...data.steps.map(s => s.version) + ) + this.#syncService.emit('change', { sessions, version }) this.#syncService._receiveSteps(data) + this.version = version if (data.steps.length === 0) { if (!this.#initialLoadingFinished) { @@ -168,17 +175,6 @@ class PollingBackend { } else { logger.error(`[PollingBackend:fetchSteps] Network error when fetching steps, retry ${this.#fetchRetryCounter}`) } - } else if (e.response.status === 409) { - // Still apply the steps to update our version of the document - this._handleResponse(e.response) - logger.error('Conflict during file save, please resolve') - this.#syncService.emit('error', { - type: ERROR_TYPE.SAVE_COLLISSION, - data: { - outsideChange: e.response.data.outsideChange, - }, - }) - this.disconnect() } else if (e.response.status === 403) { this.#syncService.emit('error', { type: ERROR_TYPE.SOURCE_NOT_FOUND, data: {} }) this.disconnect() diff --git a/src/services/SessionApi.js b/src/services/SessionApi.js index 3c4f5c868e4..a88c311ea51 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -94,8 +94,15 @@ export class Connection { } } - sync({ version, autosaveContent, documentState, force, manualSave }) { + sync({ version }) { return axios.post(this.#url('session/sync'), { + ...this.#defaultParams, + version, + }) + } + + save({ version, autosaveContent, documentState, force, manualSave }) { + return axios.post(this.#url('session/save'), { ...this.#defaultParams, filePath: this.#options.filePath, version, diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 2fae170f46a..d785962b7bf 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -75,7 +75,6 @@ class SyncService { this.lastStepPush = Date.now() - this.version = null this.sending = false this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL) @@ -91,14 +90,11 @@ class SyncService { : await this._api.open({ fileId }) .catch(error => this._emitError(error)) - this.version = this.connection.lastSavedVersion this.emit('opened', { ...this.connection.state, - version: this.version, }) this.emit('loaded', { ...this.connection.state, - version: this.version, }) this.backend = new PollingBackend(this, this.connection) @@ -143,7 +139,7 @@ class SyncService { .then((response) => { this.sending = false }).catch(({ response, code }) => { - logger.error('failed to apply steps due to collission, retrying') + logger.error('Failed to apply steps due to error communicating with server.') this.sending = false if (!response || code === 'ECONNABORTED') { this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} }) @@ -153,12 +149,7 @@ class SyncService { if (status === 403) { if (!data.document) { // either the session is invalid or the document is read only. - logger.error('failed to write to document - not allowed') - } - // Only emit conflict event if we have synced until the latest version - if (data.document?.currentVersion === this.version) { - this.emit('error', { type: ERROR_TYPE.PUSH_FAILURE, data: {} }) - OC.Notification.showTemporary('Changes could not be sent yet') + logger.error('Failed to write to document - not allowed') } } // TODO: Retry and warn @@ -174,9 +165,6 @@ class SyncService { const newSteps = awareness for (let i = 0; i < steps.length; i++) { const singleSteps = steps[i].data - if (this.version < steps[i].version) { - this.version = steps[i].version - } if (!Array.isArray(singleSteps)) { logger.error('Invalid step data, skipping step', { step: steps[i] }) // TODO: recover @@ -194,7 +182,6 @@ class SyncService { steps: newSteps, // TODO: do we actually need to dig into the connection here? document: this.connection.document, - version: this.version, }) } @@ -212,11 +199,11 @@ class SyncService { return this.serialize() } - async save({ force = false, manualSave = true } = {}) { + async save(version, { force = false, manualSave = true } = {}) { logger.debug('[SyncService] saving', arguments[0]) try { - const response = await this.connection.sync({ - version: this.version, + const response = await this.connection.save({ + version: this.backend.version, autosaveContent: this._getContent(), documentState: this.getDocumentState(), force, @@ -224,20 +211,33 @@ class SyncService { }) this.emit('stateChange', { dirty: false }) logger.debug('[SyncService] saved', response) - const { document, sessions } = response.data - this.emit('save', { document, sessions }) + this.emit('save') + this.emit('change', { document: response.data.document }) this.autosave.clear() } catch (e) { - logger.error('Failed to save document.', { error: e }) + if (e.response.status === 409) { + + // Still apply the steps to update our version of the document + // TODO: this is not possible anymore - do we need it? this._handleResponse(e.response) + // we also used to disconnect the polling backend. + logger.error('Conflict during file save, please resolve') + this.emit('error', { + type: ERROR_TYPE.SAVE_COLLISSION, + data: { + outsideChange: e.response.data.outsideChange, + }, + }) + logger.error('Failed to save document.', { error: e }) + } } } - forceSave() { - return this.save({ force: true }) + forceSave(version) { + return this.save(version, { force: true }) } - _autosave() { - return this.save({ manualSave: false }) + _autosave(version) { + return this.save(version, { manualSave: false }) } close() {