Skip to content

Commit

Permalink
[FIX] collaborative: don't send duplicate messages
Browse files Browse the repository at this point in the history
When a message is acknowledged, the `waitingAck` flag is reset to `false`,
but it's not set to `true` when the next pending message is sent.

It means that if a third local change occurs, it will send the next pending
message even though the second one is not acknowledged yet.

closes #5093

Task: 4255949
Signed-off-by: Pierre Rousseau (pro) <[email protected]>
  • Loading branch information
LucasLefevre committed Oct 21, 2024
1 parent 50e9e33 commit 122c443
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/collaborative/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,6 @@ export class Session extends EventBus<CollaborativeEvent> {
if (this.waitingAck) {
return;
}
this.waitingAck = true;
this.sendPendingMessage();
}

Expand Down Expand Up @@ -403,6 +402,7 @@ export class Session extends EventBus<CollaborativeEvent> {
throw new Error(`Trying to send a new revision while replaying initial revision. This can lead to endless dispatches every time the spreadsheet is open.
${JSON.stringify(message)}`);
}
this.waitingAck = true;
this.transportService.sendMessage({
...message,
serverRevisionId: this.serverRevisionId,
Expand Down
20 changes: 20 additions & 0 deletions tests/collaborative/collaborative.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,26 @@ describe("Multi users synchronisation", () => {
);
});

test("do not send message while waiting an acknowledgement", () => {
const spy = jest.spyOn(network, "sendMessage");
network.concurrent(() => {
setCellContent(alice, "A1", "hello");
expect(spy).toHaveBeenCalledTimes(1); // send the first revision

setCellContent(alice, "A2", "hello");
expect(spy).toHaveBeenCalledTimes(1); // do not send the second revision because the first one is not acknowledged

// we simulate the server is sending the first message
// back to the client, which acknowledge it.
// It should send the second message to the server
network.notifyListeners(network["pendingMessages"][0]); // acknowledge the first message
expect(spy).toHaveBeenCalledTimes(2); // the second message is sent
setCellContent(alice, "A3", "hello");
expect(spy).toHaveBeenCalledTimes(2); // do not send any message because the second one is not acknowledged
});
expect(spy).toHaveBeenCalledTimes(3);
});

describe("Table style", () => {
test("Create a table with a style, and delete the style at the same time", () => {
createTableStyle(alice, "MyStyle");
Expand Down

0 comments on commit 122c443

Please sign in to comment.