Skip to content

Commit

Permalink
[FIX] session: avoid concurrent updates
Browse files Browse the repository at this point in the history
Steps to reproduce (in odoo)
- Open a spreadsheet
- do something such that there's a least one revisions
- leave the spreadsheet

=> non-deterministic concurrent update because we save the thumbnail and we
snapshot at the same time.

I'm able to reproduce more when my laptop power setting is on "performance"
compared to "balanced"

With this commit `leaveSession` is now asynchronous and returns a promise
which resolves after the snapshot was done.
It allows to await this promise to trigger other RPC updating the same
record.

closes #4929

Task: 4080148
X-original-commit: ac3a12c
Signed-off-by: Pierre Rousseau (pro) <[email protected]>
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
LucasLefevre committed Sep 2, 2024
1 parent 82c59e6 commit 1a7d2d9
Show file tree
Hide file tree
Showing 6 changed files with 12 additions and 10 deletions.
2 changes: 1 addition & 1 deletion src/collaborative/local_transport_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
export class LocalTransportService implements TransportService<CollaborationMessage> {
private listeners: { id: UID; callback: NewMessageCallback }[] = [];

sendMessage(message: CollaborationMessage) {
async sendMessage(message: CollaborationMessage) {
for (const { callback } of this.listeners) {
callback(message);
}
Expand Down
8 changes: 4 additions & 4 deletions src/collaborative/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,9 @@ export class Session extends EventBus<CollaborativeEvent> {
/**
* Notify the server that the user client left the collaborative session
*/
leave(data: Lazy<WorkbookData>) {
async leave(data: Lazy<WorkbookData>) {
if (Object.keys(this.clients).length === 1 && this.processedRevisions.size) {
this.snapshot(data());
await this.snapshot(data());
}
delete this.clients[this.clientId];
this.transportService.leave(this.clientId);
Expand All @@ -181,12 +181,12 @@ export class Session extends EventBus<CollaborativeEvent> {
/**
* Send a snapshot of the spreadsheet to the collaboration server
*/
snapshot(data: WorkbookData) {
async snapshot(data: WorkbookData) {
if (this.pendingMessages.length !== 0) {
return;
}
const snapshotId = this.uuidGenerator.uuidv4();
this.transportService.sendMessage({
await this.transportService.sendMessage({
type: "SNAPSHOT",
nextRevisionId: snapshotId,
serverRevisionId: this.serverRevisionId,
Expand Down
4 changes: 2 additions & 2 deletions src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@ export class Model extends EventBus<any> implements CommandDispatcher {
this.session.join(this.config.client);
}

leaveSession() {
this.session.leave(lazy(() => this.exportData()));
async leaveSession() {
await this.session.leave(lazy(() => this.exportData()));
}

private setupUiPlugin(Plugin: UIPluginConstructor) {
Expand Down
2 changes: 1 addition & 1 deletion src/types/collaborative/transport_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export interface TransportService<T = any> {
/**
* Send a message to all clients
*/
sendMessage: (message: T) => void;
sendMessage: (message: T) => Promise<void>;
/**
* Register a callback function which will be called each time
* a new message is received.
Expand Down
2 changes: 1 addition & 1 deletion tests/__mocks__/transport_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export class MockTransportService implements TransportService<CollaborationMessa
this.listeners.push({ id, callback });
}

sendMessage(message: CollaborationMessage) {
async sendMessage(message: CollaborationMessage) {
const msg: CollaborationMessage = deepCopy(message);
switch (msg.type) {
case "REMOTE_REVISION":
Expand Down
4 changes: 3 additions & 1 deletion tests/collaborative/collaborative_session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { buildRevisionLog } from "../../src/history/factory";
import { Client, CommandResult, WorkbookData } from "../../src/types";
import { MockTransportService } from "../__mocks__/transport_service";
import { selectCell, setCellContent } from "../test_helpers/commands_helpers";
import { nextTick } from "../test_helpers/helpers";

describe("Collaborative session", () => {
let transport: MockTransportService;
Expand Down Expand Up @@ -82,7 +83,7 @@ describe("Collaborative session", () => {
});
});

test("do not snapshot when leaving if there are pending change", () => {
test("do not snapshot when leaving if there are pending change", async () => {
const model = new Model(
{},
{
Expand All @@ -98,6 +99,7 @@ describe("Collaborative session", () => {
// and leave before receiving the acknowledgement
model.leaveSession();
});
await nextTick();
expect(spy).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenCalledWith(expect.objectContaining({ type: "REMOTE_REVISION" }));
expect(spy).toHaveBeenCalledWith(expect.objectContaining({ type: "CLIENT_LEFT" }));
Expand Down

0 comments on commit 1a7d2d9

Please sign in to comment.