diff --git a/src/modules/exdraw/managers/excalidraw-manager.js b/src/modules/exdraw/managers/excalidraw-manager.js index 9f36b99..a3e0077 100644 --- a/src/modules/exdraw/managers/excalidraw-manager.js +++ b/src/modules/exdraw/managers/excalidraw-manager.js @@ -114,10 +114,18 @@ class ExcalidrawManager { } const sceneData = new ExcalidrawDocument(exdrawUuid, exdrawName, docContent); sceneData.setLastModifyUser({ username }); + + // Multiple operations may load the same cold document concurrently. + // Use the document that won the cache race instead of overwriting it. + const cachedDocument = this.documents.get(exdrawUuid); + if (cachedDocument) { + return cachedDocument.toJson(); + } + this.documents.set(exdrawUuid, sceneData); if (!result.data) { - sceneData.setMeta({need_save: true}); + sceneData.setMeta({ need_save: true }); await this.saveSceneDoc(exdrawUuid); } @@ -140,7 +148,9 @@ class ExcalidrawManager { document.setMeta({ is_saving: true }); - // Get save info + // Capture the version included in this save. Operations that arrive while + // the upload is in flight must keep the document dirty for a later save. + const savedVersion = document.version; const exdrawContent = document.toJson(); const exdrawName = document.exdrawName; @@ -165,17 +175,22 @@ class ExcalidrawManager { } finally { deleteDir(tempPath); } - document.setMeta({is_saving: false, need_save: false}); + const hasNewerChanges = document.version !== savedVersion; + document.setMeta({ + is_saving: false, + need_save: !saveFlag || hasNewerChanges, + }); return Promise.resolve(saveFlag); }; saveSceneDocToMemory = async (exdrawUuid, exdrawName, content, username) => { - const document = this.documents.get(exdrawUuid); + let document = this.documents.get(exdrawUuid); if (!document) { try { // Load the document before executing op to avoid the document not being loaded into the memory after disconnection and reconnection - await this.getDoc(exdrawUuid, exdrawName); + await this.getSceneDoc(exdrawUuid, exdrawName); + document = this.documents.get(exdrawUuid); } catch(e) { logger.error(`SOCKET_MESSAGE: Load ${exdrawName}(${exdrawUuid}) doc content error`); const result = { @@ -186,6 +201,14 @@ class ExcalidrawManager { } } + if (!document) { + logger.error(`SOCKET_MESSAGE: Document ${exdrawName}(${exdrawUuid}) is not available after loading`); + return Promise.resolve({ + success: false, + error_type: 'load_document_content_error', + }); + } + const { version: clientVersion} = content; const { version: serverVersion, elements } = document; if (serverVersion !== clientVersion) { @@ -214,11 +237,12 @@ class ExcalidrawManager { execOperationsBySocket = async (params, exdrawName) => { const { doc_uuid: docUuid, version: clientVersion, user, elements } = params; - const document = this.documents.get(docUuid); + let document = this.documents.get(docUuid); if (!document) { try { // Load the document before executing op to avoid the document not being loaded into the memory after disconnection and reconnection - await this.getDoc(docUuid, exdrawName); + await this.getSceneDoc(docUuid, exdrawName); + document = this.documents.get(docUuid); } catch(e) { logger.error(`SOCKET_MESSAGE: Load ${exdrawName}(${docUuid}) doc content error`); const result = { @@ -229,6 +253,14 @@ class ExcalidrawManager { } } + if (!document) { + logger.error(`SOCKET_MESSAGE: Document ${exdrawName}(${docUuid}) is not available after loading`); + return Promise.resolve({ + success: false, + error_type: 'load_document_content_error', + }); + } + const { version: serverVersion } = document; if (serverVersion !== clientVersion) { const result = { @@ -242,6 +274,14 @@ class ExcalidrawManager { return Promise.resolve(result); } + if (!Array.isArray(elements) || elements.length === 0) { + return Promise.resolve({ + success: true, + updated: false, + version: document.version, + }); + } + // execute operations success let isExecuteSuccess = false; try { diff --git a/src/modules/exdraw/wio/io-handler.js b/src/modules/exdraw/wio/io-handler.js index bb4a4a2..c6b7a90 100644 --- a/src/modules/exdraw/wio/io-handler.js +++ b/src/modules/exdraw/wio/io-handler.js @@ -2,6 +2,7 @@ import ExcalidrawManager from "../managers/excalidraw-manager"; import UsersManager from "../managers/users-manager"; import IOHelper from "./io-helper"; import checkPermission from "./is-permission-valid"; +import logger from "../../../loggers"; class ExdrawIOHandler { @@ -24,25 +25,37 @@ class ExdrawIOHandler { onConnection(socket) { // todo permission check this.ioHelper.sendInitRoomToPrivate(socket.id); - socket.on('join-room', async (params) => { - // join room - const { doc_uuid: docUuid, user: userInfo } = params; - socket.join(docUuid); - - const usersManager = UsersManager.getInstance(); - if (!usersManager.getUser(docUuid, socket.id)) { - usersManager.addUser(docUuid, socket.id, userInfo); + socket.on('join-room', async (params, callback) => { + try { + const { doc_uuid: docUuid, user: userInfo } = params || {}; + if (!docUuid || !userInfo) { + throw new Error('Invalid join-room parameters'); + } + + await socket.join(docUuid); + + const usersManager = UsersManager.getInstance(); + if (!usersManager.getUser(docUuid, socket.id)) { + usersManager.addUser(docUuid, socket.id, userInfo); + } + + const users = usersManager.getDocUsers(docUuid); + + // if (users.length === 1) { + // this.ioHelper.sendFirstInRoomMessage(socket.id); + // } else { + // this.ioHelper.sendNewUserMessage(socket, docUuid); + // } + + this.ioHelper.sendRoomUserChangeMessage(socket, docUuid, users); + callback && callback({ success: true }); + } catch (error) { + logger.error('SOCKET_MESSAGE: Join room failed', error); + callback && callback({ + success: false, + error_type: 'join_room_error', + }); } - - const users = usersManager.getDocUsers(docUuid); - - // if (users.length === 1) { - // this.ioHelper.sendFirstInRoomMessage(socket.id); - // } else { - // this.ioHelper.sendNewUserMessage(socket, docUuid); - // } - - this.ioHelper.sendRoomUserChangeMessage(socket, docUuid, users); }); socket.on('elements-updated', async (params, callback) => { diff --git a/tests/managers/excalidraw-manager.test.js b/tests/managers/excalidraw-manager.test.js new file mode 100644 index 0000000..cdae427 --- /dev/null +++ b/tests/managers/excalidraw-manager.test.js @@ -0,0 +1,102 @@ +import ExcalidrawManager from '../../src/modules/exdraw/managers/excalidraw-manager'; +import ExcalidrawDocument from '../../src/modules/exdraw/models/excalidraw-document'; +import seaServerAPI from '../../src/modules/exdraw/api/sea-server-api'; + +jest.mock('../../src/modules/exdraw/api/sea-server-api', () => ({ + __esModule: true, + default: { + getSceneDownloadLink: jest.fn(), + getSceneContent: jest.fn(), + saveSceneContent: jest.fn(), + }, +})); + +jest.mock('../../src/loggers', () => ({ + __esModule: true, + default: { + error: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + }, +})); + +jest.mock('../../src/utils', () => ({ + __esModule: true, + deleteDir: jest.fn(), + getErrorMessage: jest.fn(() => ({})), + errorHandle: jest.fn(), +})); + +const createDeferred = () => { + let resolve; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +}; + +describe('ExcalidrawManager cold document loading and saving', () => { + let manager; + + beforeEach(() => { + manager = ExcalidrawManager.getInstance(); + manager.documents = new Map(); + jest.clearAllMocks(); + seaServerAPI.getSceneDownloadLink.mockResolvedValue({ + data: { download_link: 'https://example.test/doc' }, + }); + }); + + it('uses the document already cached when concurrent cold loads finish', async () => { + const contentLoads = []; + seaServerAPI.getSceneContent.mockImplementation(() => { + const deferred = createDeferred(); + contentLoads.push(deferred); + return deferred.promise; + }); + + const firstLoad = manager.getSceneDoc('doc-uuid', 'doc.excalidraw', 'user-a'); + const secondLoad = manager.getSceneDoc('doc-uuid', 'doc.excalidraw', 'user-b'); + + await new Promise(resolve => setImmediate(resolve)); + expect(contentLoads).toHaveLength(2); + + contentLoads[0].resolve({ + data: { version: 0, elements: [] }, + }); + await new Promise(resolve => setImmediate(resolve)); + + contentLoads[1].resolve({ + data: { version: 0, elements: [] }, + }); + + const [firstDocument, secondDocument] = await Promise.all([firstLoad, secondLoad]); + const cachedDocument = manager.documents.get('doc-uuid'); + + expect(cachedDocument.last_modify_user).toBe('user-a'); + expect(firstDocument.last_modify_user).toBe('user-a'); + expect(secondDocument.last_modify_user).toBe('user-a'); + }); + + it('keeps the document dirty when an operation arrives during saving', async () => { + const document = new ExcalidrawDocument('doc-uuid', 'doc.excalidraw', { + version: 0, + elements: [], + }); + document.setMeta({ need_save: true }); + manager.documents.set('doc-uuid', document); + + const save = createDeferred(); + seaServerAPI.saveSceneContent.mockReturnValue(save.promise); + + const savePromise = manager.saveSceneDoc('doc-uuid'); + document.setValue([{ id: 'new-element' }], 1); + save.resolve({ data: {} }); + + expect(await savePromise).toBe(true); + expect(document.getMeta()).toEqual(expect.objectContaining({ + is_saving: false, + need_save: true, + })); + }); +}); diff --git a/tests/wio-io-handler.test.js b/tests/wio-io-handler.test.js new file mode 100644 index 0000000..e9b2af8 --- /dev/null +++ b/tests/wio-io-handler.test.js @@ -0,0 +1,92 @@ +import ExdrawIOHandler from '../src/modules/exdraw/wio/io-handler'; +import IOHelper from '../src/modules/exdraw/wio/io-helper'; +import UsersManager from '../src/modules/exdraw/managers/users-manager'; + +jest.mock('../src/modules/exdraw/wio/io-helper'); +jest.mock('../src/modules/exdraw/managers/users-manager'); +jest.mock('../src/loggers', () => ({ + __esModule: true, + default: { + error: jest.fn(), + }, +})); + +describe('ExdrawIOHandler join-room', () => { + let ioHelper; + let usersManager; + let socket; + let handlers; + + beforeEach(() => { + handlers = {}; + ioHelper = { + sendInitRoomToPrivate: jest.fn(), + sendRoomUserChangeMessage: jest.fn(), + }; + usersManager = { + getUser: jest.fn(), + addUser: jest.fn(), + getDocUsers: jest.fn().mockReturnValue([]), + }; + socket = { + id: 'socket-id', + on: jest.fn((event, handler) => { + handlers[event] = handler; + }), + join: jest.fn().mockResolvedValue(undefined), + }; + IOHelper.getInstance.mockReturnValue(ioHelper); + UsersManager.getInstance.mockReturnValue(usersManager); + }); + + it('returns a success acknowledgement after joining the room', async () => { + const callback = jest.fn(); + const handler = new ExdrawIOHandler({}); + + handler.onConnection(socket); + await handlers['join-room']({ + doc_uuid: 'doc-uuid', + user: { _username: 'user' }, + }, callback); + + expect(socket.join).toHaveBeenCalledWith('doc-uuid'); + expect(ioHelper.sendRoomUserChangeMessage).toHaveBeenCalledWith(socket, 'doc-uuid', []); + expect(callback).toHaveBeenCalledWith({ success: true }); + }); + + it('returns a failure acknowledgement when joining the room throws', async () => { + const callback = jest.fn(); + socket.join.mockRejectedValue(new Error('adapter unavailable')); + const handler = new ExdrawIOHandler({}); + + handler.onConnection(socket); + await handlers['join-room']({ + doc_uuid: 'doc-uuid', + user: { _username: 'user' }, + }, callback); + + expect(callback).toHaveBeenCalledWith({ + success: false, + error_type: 'join_room_error', + }); + }); + + it('returns a failure acknowledgement when user management throws', async () => { + const callback = jest.fn(); + usersManager.addUser.mockImplementation(() => { + throw new Error('user manager unavailable'); + }); + const handler = new ExdrawIOHandler({}); + + handler.onConnection(socket); + await handlers['join-room']({ + doc_uuid: 'doc-uuid', + user: { _username: 'user' }, + }, callback); + + expect(callback).toHaveBeenCalledWith({ + success: false, + error_type: 'join_room_error', + }); + }); +});