From f985772afcd2b1b118028cf9cfb826c7e7e1daf1 Mon Sep 17 00:00:00 2001 From: Eric Maciel Date: Wed, 3 Mar 2021 12:16:50 -0500 Subject: [PATCH 1/5] fix: large collab ops --- .../backend/src/AutomergeCollaboration.ts | 11 ++--- .../backend/src/utils/getActiveConnections.ts | 12 +++++ packages/client/src/client.spec.ts | 44 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 packages/backend/src/utils/getActiveConnections.ts diff --git a/packages/backend/src/AutomergeCollaboration.ts b/packages/backend/src/AutomergeCollaboration.ts index 5452ab6..c529f6c 100644 --- a/packages/backend/src/AutomergeCollaboration.ts +++ b/packages/backend/src/AutomergeCollaboration.ts @@ -7,6 +7,7 @@ import flatten from 'lodash/flatten' import { SyncDoc, CollabAction, toJS } from '@hiveteams/collab-bridge' import { debugCollabBackend } from './utils/debug' import AutomergeBackend from './AutomergeBackend' +import getActiveConnections from './utils/getActiveConnections' export interface IAutomergeMetaData { docId: string @@ -253,6 +254,10 @@ export default class AutomergeCollaboration { opCount: collabActions.length } + if (collabActions.length > 10) { + console.log(JSON.stringify(data.payload.changes, null, 2)) + } + this.onTrace(metaData, () => { switch (data.type) { case 'operation': @@ -344,11 +349,7 @@ export default class AutomergeCollaboration { garbageNsp = (socket: SocketIO.Socket) => { const { name: docId } = socket.nsp - // This is the only way to synchronously check the number of active Automerge.Connections - // for this docId. - // @ts-ignore - const activeConnectionsCount = this.backend.documentSetMap[docId]?.handlers - .size + const activeConnectionsCount = getActiveConnections(this.backend, docId) debugCollabBackend( 'Garbage namespace activeConnections=%s', diff --git a/packages/backend/src/utils/getActiveConnections.ts b/packages/backend/src/utils/getActiveConnections.ts new file mode 100644 index 0000000..e6b5f42 --- /dev/null +++ b/packages/backend/src/utils/getActiveConnections.ts @@ -0,0 +1,12 @@ +import AutomergeBackend from '../AutomergeBackend' + +const getActiveConnections = (backend: AutomergeBackend, docId: string) => { + // This is the only way to synchronously check the number of active Automerge.Connections + // for this docId. + // @ts-ignore + const activeConnectionsCount = backend.documentSetMap[docId]?.handlers.size + + return activeConnectionsCount +} + +export default getActiveConnections diff --git a/packages/client/src/client.spec.ts b/packages/client/src/client.spec.ts index d09dce9..b0eb16b 100644 --- a/packages/client/src/client.spec.ts +++ b/packages/client/src/client.spec.ts @@ -8,6 +8,7 @@ import AutomergeCollaboration from '@hiveteams/collab-backend/lib/AutomergeColla import withIOCollaboration from './withIOCollaboration' import { AutomergeOptions, SocketIOPluginOptions } from './interfaces' import { getTarget } from '@hiveteams/collab-bridge/src/path' +import getActiveConnections from '@hiveteams/collab-backend/src/utils/getActiveConnections' const connectionSlug = 'test' const docId = `/${connectionSlug}` @@ -50,6 +51,12 @@ const collabBackend = new AutomergeCollaboration({ }, async onDocumentLoad(pathname) { return defaultSlateJson + }, + onTrace(metaData, computationFn) { + if (metaData.opCount && metaData.opCount > 100) { + } + console.log('metaData', metaData) + computationFn() } }) @@ -60,6 +67,10 @@ describe('automerge editor client tests', () => { server.listen(5000, () => done()) }) + afterEach(done => { + waitForCondition(() => !collabBackend.backend.getDocument(docId)).then(done) + }) + const createCollabEditor = async ( editorOptions?: Partial & Partial ) => { @@ -232,6 +243,8 @@ describe('automerge editor client tests', () => { expect(editor.children.length).toEqual(2) expect(Node.string(editor.children[0])).toEqual('new') expect(Node.string(editor.children[1])).toEqual('nodes') + + editor.destroy() }) it('set node for children with missing value should not throw error', () => { @@ -265,6 +278,37 @@ describe('automerge editor client tests', () => { expect(target).toEqual(null) }) + it('should work with concurrent insert text operations', async () => { + const editor1 = await createCollabEditor() + const editor2 = await createCollabEditor() + + editor1.disconnect() + + await waitForCondition(() => { + return getActiveConnections(collabBackend.backend, docId) === 1 + }) + + editor2.insertNode({ type: 'paragraph', children: [{ text: 'hi' }] }) + + await waitForCondition(() => { + return collabBackend.backend.getDocument(docId)?.children.length === 2 + }) + + editor1.connect() + + await waitForCondition(() => { + return editor1.children.length === 2 + }) + + editor2.destroy() + + await waitForCondition(() => { + return getActiveConnections(collabBackend.backend, docId) === 1 + }) + + editor1.destroy() + }) + afterAll(() => { collabBackend.destroy() server.close() From 513770336a67a304ae292dea9ea58b755339a0fb Mon Sep 17 00:00:00 2001 From: Eric Maciel Date: Wed, 3 Mar 2021 12:53:03 -0500 Subject: [PATCH 2/5] test: clean initial test state --- .../backend/src/AutomergeCollaboration.ts | 4 +-- .../backend/src/utils/getActiveConnections.ts | 8 +++--- packages/client/src/client.spec.ts | 27 ++++++++++++------- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/backend/src/AutomergeCollaboration.ts b/packages/backend/src/AutomergeCollaboration.ts index c529f6c..a7e4ceb 100644 --- a/packages/backend/src/AutomergeCollaboration.ts +++ b/packages/backend/src/AutomergeCollaboration.ts @@ -254,9 +254,7 @@ export default class AutomergeCollaboration { opCount: collabActions.length } - if (collabActions.length > 10) { - console.log(JSON.stringify(data.payload.changes, null, 2)) - } + console.log(JSON.stringify(data.payload, null, 2)) this.onTrace(metaData, () => { switch (data.type) { diff --git a/packages/backend/src/utils/getActiveConnections.ts b/packages/backend/src/utils/getActiveConnections.ts index e6b5f42..bc85d45 100644 --- a/packages/backend/src/utils/getActiveConnections.ts +++ b/packages/backend/src/utils/getActiveConnections.ts @@ -1,12 +1,14 @@ import AutomergeBackend from '../AutomergeBackend' const getActiveConnections = (backend: AutomergeBackend, docId: string) => { + const automergeDocument = backend.documentSetMap[docId] + + if (!automergeDocument) return 0 + // This is the only way to synchronously check the number of active Automerge.Connections // for this docId. // @ts-ignore - const activeConnectionsCount = backend.documentSetMap[docId]?.handlers.size - - return activeConnectionsCount + return automergeDocument.handlers.size } export default getActiveConnections diff --git a/packages/client/src/client.spec.ts b/packages/client/src/client.spec.ts index b0eb16b..f84f5c6 100644 --- a/packages/client/src/client.spec.ts +++ b/packages/client/src/client.spec.ts @@ -278,7 +278,7 @@ describe('automerge editor client tests', () => { expect(target).toEqual(null) }) - it('should work with concurrent insert text operations', async () => { + it.only('should work with concurrent insert text operations', async () => { const editor1 = await createCollabEditor() const editor2 = await createCollabEditor() @@ -288,17 +288,20 @@ describe('automerge editor client tests', () => { return getActiveConnections(collabBackend.backend, docId) === 1 }) - editor2.insertNode({ type: 'paragraph', children: [{ text: 'hi' }] }) - - await waitForCondition(() => { - return collabBackend.backend.getDocument(docId)?.children.length === 2 - }) + // editor2.insertNode({ type: 'paragraph', children: [{ text: 'hi' }] }) + // await waitForCondition(() => { + // return collabBackend.backend.getDocument(docId)?.children.length === 1 + // }) editor1.connect() - await waitForCondition(() => { - return editor1.children.length === 2 - }) + await waitForCondition( + () => getActiveConnections(collabBackend.backend, docId) === 2 + ) + + // await waitForCondition(() => { + // return editor1.children.length === 1 + // }) editor2.destroy() @@ -306,7 +309,13 @@ describe('automerge editor client tests', () => { return getActiveConnections(collabBackend.backend, docId) === 1 }) + console.log('destroying last editor') + editor1.destroy() + + await waitForCondition(() => { + return getActiveConnections(collabBackend.backend, docId) === 0 + }) }) afterAll(() => { From 581d68b14229059bfcbe2674a3a3293379231861 Mon Sep 17 00:00:00 2001 From: schulace Date: Wed, 3 Mar 2021 14:24:36 -0700 Subject: [PATCH 3/5] chore: minimal reproduction of large diff on reconnect --- packages/client/package.json | 2 +- packages/client/src/client.spec.ts | 37 +++++++++++++----------------- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/packages/client/package.json b/packages/client/package.json index 5df9c8e..2bbea51 100644 --- a/packages/client/package.json +++ b/packages/client/package.json @@ -22,7 +22,7 @@ "build:types": "tsc --emitDeclarationOnly", "build:js": "babel src --out-dir lib --extensions \".ts,.tsx\" --source-maps inline", "watch": "yarn build:js -w", - "test": "jest" + "test": "DEBUG=app* jest" }, "dependencies": { "@babel/plugin-proposal-optional-chaining": "^7.9.0", diff --git a/packages/client/src/client.spec.ts b/packages/client/src/client.spec.ts index f84f5c6..cae3ae1 100644 --- a/packages/client/src/client.spec.ts +++ b/packages/client/src/client.spec.ts @@ -41,7 +41,12 @@ const server = createServer(function(req, res) { res.end() }) -const defaultSlateJson = [{ type: 'paragraph', children: [{ text: '' }] }] +const defaultSlateJson = [ + { + type: 'paragraph', + children: [{ text: 'hello world' }, { text: 'goodbye world' }] + } +] const collabBackend = new AutomergeCollaboration({ entry: server, defaultValue: defaultSlateJson, @@ -280,37 +285,27 @@ describe('automerge editor client tests', () => { it.only('should work with concurrent insert text operations', async () => { const editor1 = await createCollabEditor() - const editor2 = await createCollabEditor() + console.log('----\neditor1 disconnect\n-----') + await waitForCondition(() => { + return getActiveConnections(collabBackend.backend, docId) === 1 + }) editor1.disconnect() - await waitForCondition(() => { - return getActiveConnections(collabBackend.backend, docId) === 1 - }) - - // editor2.insertNode({ type: 'paragraph', children: [{ text: 'hi' }] }) - // await waitForCondition(() => { - // return collabBackend.backend.getDocument(docId)?.children.length === 1 - // }) + await waitForCondition( + () => getActiveConnections(collabBackend.backend, docId) === 0 + ) + console.log('----\neditor1 reconnect\n-----') editor1.connect() await waitForCondition( - () => getActiveConnections(collabBackend.backend, docId) === 2 + () => getActiveConnections(collabBackend.backend, docId) === 1 ) - // await waitForCondition(() => { - // return editor1.children.length === 1 - // }) - - editor2.destroy() - - await waitForCondition(() => { - return getActiveConnections(collabBackend.backend, docId) === 1 - }) + await new Promise(res => setTimeout(res, 3000)) console.log('destroying last editor') - editor1.destroy() await waitForCondition(() => { From d0a793048405a6c2c4a0864cbc65b5723798fb3f Mon Sep 17 00:00:00 2001 From: Eric Maciel Date: Thu, 4 Mar 2021 15:37:10 -0500 Subject: [PATCH 4/5] feat: add ability to reset docSet for client on reconnect (resetOnReconnect) --- .../backend/src/AutomergeCollaboration.ts | 2 - packages/client/src/client.spec.ts | 64 +++++++++---------- packages/client/src/interfaces.ts | 1 + packages/client/src/withSocketIO.ts | 20 +++++- 4 files changed, 49 insertions(+), 38 deletions(-) diff --git a/packages/backend/src/AutomergeCollaboration.ts b/packages/backend/src/AutomergeCollaboration.ts index a7e4ceb..2fe970e 100644 --- a/packages/backend/src/AutomergeCollaboration.ts +++ b/packages/backend/src/AutomergeCollaboration.ts @@ -254,8 +254,6 @@ export default class AutomergeCollaboration { opCount: collabActions.length } - console.log(JSON.stringify(data.payload, null, 2)) - this.onTrace(metaData, () => { switch (data.type) { case 'operation': diff --git a/packages/client/src/client.spec.ts b/packages/client/src/client.spec.ts index cae3ae1..53d04cf 100644 --- a/packages/client/src/client.spec.ts +++ b/packages/client/src/client.spec.ts @@ -4,9 +4,16 @@ import fs from 'fs' import isEqual from 'lodash/isEqual' import { createEditor, Editor, Element, Node, Transforms } from 'slate' import { createDoc, SyncDoc, toJS, toSlateOp } from '@hiveteams/collab-bridge' -import AutomergeCollaboration from '@hiveteams/collab-backend/lib/AutomergeCollaboration' +import AutomergeCollaboration, { + IAutomergeMetaData +} from '@hiveteams/collab-backend/lib/AutomergeCollaboration' import withIOCollaboration from './withIOCollaboration' -import { AutomergeOptions, SocketIOPluginOptions } from './interfaces' +import { + AutomergeEditor, + AutomergeOptions, + SocketIOPluginOptions, + WithSocketIOEditor +} from './interfaces' import { getTarget } from '@hiveteams/collab-bridge/src/path' import getActiveConnections from '@hiveteams/collab-backend/src/utils/getActiveConnections' @@ -41,12 +48,8 @@ const server = createServer(function(req, res) { res.end() }) -const defaultSlateJson = [ - { - type: 'paragraph', - children: [{ text: 'hello world' }, { text: 'goodbye world' }] - } -] +const defaultSlateJson = [{ type: 'paragraph', children: [{ text: '' }] }] +let operationTraces: IAutomergeMetaData[] = [] const collabBackend = new AutomergeCollaboration({ entry: server, defaultValue: defaultSlateJson, @@ -58,9 +61,7 @@ const collabBackend = new AutomergeCollaboration({ return defaultSlateJson }, onTrace(metaData, computationFn) { - if (metaData.opCount && metaData.opCount > 100) { - } - console.log('metaData', metaData) + operationTraces.push(metaData) computationFn() } }) @@ -72,7 +73,12 @@ describe('automerge editor client tests', () => { server.listen(5000, () => done()) }) + let collabEditors: (Editor & WithSocketIOEditor & AutomergeEditor)[] = [] afterEach(done => { + operationTraces = [] + collabEditors.forEach(editor => editor.destroy()) + collabEditors = [] + waitForCondition(() => !collabBackend.backend.getDocument(docId)).then(done) }) @@ -98,6 +104,7 @@ describe('automerge editor client tests', () => { }) editor.connect() + collabEditors.push(editor) await promise return editor } @@ -117,8 +124,6 @@ describe('automerge editor client tests', () => { const serverDoc = toJS(collabBackend.backend.getDocument(docId)) return serverDoc.children.length === 2 }) - - editor.destroy() }) it('should sync updates across two clients', async () => { @@ -131,9 +136,6 @@ describe('automerge editor client tests', () => { const serverDoc = toJS(collabBackend.backend.getDocument(docId)) return serverDoc.children.length === 2 && editor2.children.length === 2 }) - - editor1.destroy() - editor2.destroy() }) it('should sync offline changes on reconnect', async () => { @@ -159,9 +161,6 @@ describe('automerge editor client tests', () => { }) expect(Node.string(editor2.children[2])).toEqual('offline') - - editor1.destroy() - editor2.destroy() }) it('should work with concurrent edits', async () => { @@ -182,9 +181,6 @@ describe('automerge editor client tests', () => { }) expect(isEqual(editor1.children, editor2.children)).toBeTruthy() - - editor1.destroy() - editor2.destroy() }) it('should work with concurrent insert text operations', async () => { @@ -208,9 +204,6 @@ describe('automerge editor client tests', () => { }) expect(isEqual(editor1.children, editor2.children)).toBeTruthy() - - editor1.destroy() - editor2.destroy() }) it('should not throw deep nested tree error', () => { @@ -248,8 +241,6 @@ describe('automerge editor client tests', () => { expect(editor.children.length).toEqual(2) expect(Node.string(editor.children[0])).toEqual('new') expect(Node.string(editor.children[1])).toEqual('nodes') - - editor.destroy() }) it('set node for children with missing value should not throw error', () => { @@ -283,8 +274,8 @@ describe('automerge editor client tests', () => { expect(target).toEqual(null) }) - it.only('should work with concurrent insert text operations', async () => { - const editor1 = await createCollabEditor() + it('should reconnect with no opCount', async () => { + const editor1 = await createCollabEditor({ resetOnReconnect: true }) console.log('----\neditor1 disconnect\n-----') await waitForCondition(() => { return getActiveConnections(collabBackend.backend, docId) === 1 @@ -303,14 +294,17 @@ describe('automerge editor client tests', () => { () => getActiveConnections(collabBackend.backend, docId) === 1 ) + // Wait for a few seconds to allow the client and server to synchronize their + // document states await new Promise(res => setTimeout(res, 3000)) - console.log('destroying last editor') - editor1.destroy() - - await waitForCondition(() => { - return getActiveConnections(collabBackend.backend, docId) === 0 - }) + // Expect that reconnecting with resetOnReconnect option set to true + // does not result in any operations being sent from the client to the server + expect( + operationTraces.some( + trace => trace.opCount !== undefined && trace.opCount > 0 + ) + ).toBeFalsy }) afterAll(() => { diff --git a/packages/client/src/interfaces.ts b/packages/client/src/interfaces.ts index 6a37942..a936057 100644 --- a/packages/client/src/interfaces.ts +++ b/packages/client/src/interfaces.ts @@ -38,6 +38,7 @@ export interface SocketIOPluginOptions { onConnect?: () => void onDisconnect?: () => void onError?: (msg: string | Error, data: any) => void + resetOnReconnect?: boolean } export interface WithSocketIOEditor { diff --git a/packages/client/src/withSocketIO.ts b/packages/client/src/withSocketIO.ts index 4dad0fc..c899d7c 100644 --- a/packages/client/src/withSocketIO.ts +++ b/packages/client/src/withSocketIO.ts @@ -17,7 +17,14 @@ const withSocketIO = ( slateEditor: T, options: SocketIOPluginOptions & AutomergeOptions ) => { - const { onConnect, onDisconnect, connectOpts, url } = options + const { + onConnect, + onDisconnect, + connectOpts, + url, + docId, + resetOnReconnect + } = options const editor = slateEditor as T & WithSocketIOEditor & AutomergeEditor let socket: SocketIOClient.Socket @@ -31,6 +38,17 @@ const withSocketIO = ( // On socket io connect, open a new automerge connection socket.on('connect', () => { editor.clientId = socket.id + + // If the resetOnReconnect option is true we should close our connection + // and remove our document from the docSet if the user has already received + // a document from our collab server + if (resetOnReconnect && editor.docSet.getDoc(docId)) { + if (editor.connection) { + editor.connection.close() + } + editor.docSet.removeDoc(docId) + } + editor.openConnection() onConnect && onConnect() }) From adc7ac7dbf209744ae74b04c870f6c5acbbcc5b6 Mon Sep 17 00:00:00 2001 From: Eric Maciel Date: Thu, 4 Mar 2021 15:54:39 -0500 Subject: [PATCH 5/5] chore: code commenting --- packages/backend/src/utils/getActiveConnections.ts | 3 +++ packages/client/src/client.spec.ts | 7 +++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/utils/getActiveConnections.ts b/packages/backend/src/utils/getActiveConnections.ts index bc85d45..366eecd 100644 --- a/packages/backend/src/utils/getActiveConnections.ts +++ b/packages/backend/src/utils/getActiveConnections.ts @@ -1,5 +1,8 @@ import AutomergeBackend from '../AutomergeBackend' +/** + * Get the number of active connections for the specified docId + */ const getActiveConnections = (backend: AutomergeBackend, docId: string) => { const automergeDocument = backend.documentSetMap[docId] diff --git a/packages/client/src/client.spec.ts b/packages/client/src/client.spec.ts index 53d04cf..69237b5 100644 --- a/packages/client/src/client.spec.ts +++ b/packages/client/src/client.spec.ts @@ -75,10 +75,14 @@ describe('automerge editor client tests', () => { let collabEditors: (Editor & WithSocketIOEditor & AutomergeEditor)[] = [] afterEach(done => { + // Clear our operation traces after each test operationTraces = [] + + // Destroy any created collab editors after each test collabEditors.forEach(editor => editor.destroy()) collabEditors = [] + // Ensure that the collab document has been cleaned up on the backend waitForCondition(() => !collabBackend.backend.getDocument(docId)).then(done) }) @@ -276,7 +280,7 @@ describe('automerge editor client tests', () => { it('should reconnect with no opCount', async () => { const editor1 = await createCollabEditor({ resetOnReconnect: true }) - console.log('----\neditor1 disconnect\n-----') + await waitForCondition(() => { return getActiveConnections(collabBackend.backend, docId) === 1 }) @@ -287,7 +291,6 @@ describe('automerge editor client tests', () => { () => getActiveConnections(collabBackend.backend, docId) === 0 ) - console.log('----\neditor1 reconnect\n-----') editor1.connect() await waitForCondition(