From 7801425173475fd77c09a8cde08310268d7235dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Mon, 3 Jun 2024 15:44:11 +0200 Subject: [PATCH] [FIX] collaborative: snapshot when leaving spreadsheet Purpose ------- Let's say a user works a lot on a spreadsheet and performs a lot of heavy operations generating very large revisions (with lots of commands), such as copy-pasting a very large zone (there's one UPDATE_CELL command per copy-pasted cell) If the user does multiple such very large revision, one after the other, every thing is fine client side. The next time the spreadsheet is open though: the server needs to load all those revisions to send them to the client. The revisions can be so large it can blow up the server memory limit. The spreadsheet cannot be open anymore. Solution -------- The solution proposed here is to snapshot the spreadsheet when the client leaves the spreadsheet. Next time the spreadsheet is open, the snapshot will be loaded, and not the revivisions. There's a catch: snapshotting a spreadsheet kills the local history (CTRL+Z). For this reason, we only snapshot if there's no other connected client as it would kill the other users history. closes odoo/o-spreadsheet#4411 Task: 3940465 X-original-commit: 7bcff343946da7fbdc5e13f8b0b15f87fe0f0828 Signed-off-by: Pierre Rousseau (pro) --- src/collaborative/session.ts | 5 +- src/model.ts | 2 +- .../collaborative_session.test.ts | 58 ++++++++++++++++++- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/collaborative/session.ts b/src/collaborative/session.ts index 86881f42ff..f9391672c7 100644 --- a/src/collaborative/session.ts +++ b/src/collaborative/session.ts @@ -165,7 +165,10 @@ export class Session extends EventBus { /** * Notify the server that the user client left the collaborative session */ - leave() { + leave(data: WorkbookData) { + if (Object.keys(this.clients).length === 1 && this.processedRevisions.size) { + this.snapshot(data); + } delete this.clients[this.clientId]; this.transportService.leave(this.clientId); this.transportService.sendMessage({ diff --git a/src/model.ts b/src/model.ts index 9e563b3da0..fae979e8b0 100644 --- a/src/model.ts +++ b/src/model.ts @@ -302,7 +302,7 @@ export class Model extends EventBus implements CommandDispatcher { } leaveSession() { - this.session.leave(); + this.session.leave(this.exportData()); } private setupUiPlugin(Plugin: UIPluginConstructor) { diff --git a/tests/collaborative/collaborative_session.test.ts b/tests/collaborative/collaborative_session.test.ts index 202b60cc43..43d494b27b 100644 --- a/tests/collaborative/collaborative_session.test.ts +++ b/tests/collaborative/collaborative_session.test.ts @@ -2,7 +2,7 @@ import { Model } from "../../src"; import { Session } from "../../src/collaborative/session"; import { DEBOUNCE_TIME, MESSAGE_VERSION } from "../../src/constants"; import { buildRevisionLog } from "../../src/history/factory"; -import { Client, CommandResult } from "../../src/types"; +import { Client, CommandResult, WorkbookData } from "../../src/types"; import { MockTransportService } from "../__mocks__/transport_service"; import { selectCell } from "../test_helpers/commands_helpers"; @@ -50,7 +50,8 @@ describe("Collaborative session", () => { test("local client leaves", () => { const spy = jest.spyOn(transport, "sendMessage"); - session.leave(); + session.leave({} as WorkbookData); + expect(spy).toHaveBeenCalledTimes(1); expect(spy).toHaveBeenCalledWith({ type: "CLIENT_LEFT", version: MESSAGE_VERSION, @@ -59,6 +60,57 @@ describe("Collaborative session", () => { expect(session.getConnectedClients()).toEqual(new Set()); }); + test("local client leaves with no other clients and changes", () => { + transport.sendMessage({ + type: "REMOTE_REVISION", + version: MESSAGE_VERSION, + nextRevisionId: "42", + clientId: "client_42", + commands: [], + serverRevisionId: transport["serverRevisionId"], + }); + const spy = jest.spyOn(transport, "sendMessage"); + const data = { sheets: [{}] } as WorkbookData; + session.leave(data); + expect(spy).toHaveBeenCalledWith({ + type: "SNAPSHOT", + version: MESSAGE_VERSION, + nextRevisionId: expect.any(String), + serverRevisionId: "42", + data: { ...data, revisionId: expect.any(String) }, + }); + }); + + test("local client leaves with other connected clients and changes", () => { + transport.sendMessage({ + type: "CLIENT_JOINED", + version: MESSAGE_VERSION, + client: { + id: "bob", + name: "Bob", + position: { sheetId: "sheet1", col: 0, row: 0 }, + }, + }); + expect(session.getConnectedClients().size).toBe(2); + transport.sendMessage({ + type: "REMOTE_REVISION", + version: MESSAGE_VERSION, + nextRevisionId: "42", + clientId: "client_42", + commands: [], + serverRevisionId: transport["serverRevisionId"], + }); + const spy = jest.spyOn(transport, "sendMessage"); + const data = { sheets: [{}] } as WorkbookData; + session.leave(data); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith({ + type: "CLIENT_LEFT", + version: MESSAGE_VERSION, + clientId: client.id, + }); + }); + test("remote client move", () => { transport.sendMessage({ type: "CLIENT_MOVED", @@ -147,7 +199,7 @@ describe("Collaborative session", () => { test("Leave the session do not crash", () => { session.move({ sheetId: "sheetId", col: 1, row: 2 }); - session.leave(); + session.leave({} as WorkbookData); jest.advanceTimersByTime(DEBOUNCE_TIME + 100); });