yjs / y-websocket

Websocket Connector for Yjs
https://docs.yjs.dev/ecosystem/connection-provider/y-websocket
MIT License
523 stars 261 forks source link

BroadcastChannel client deleting Y.Map element causes delayed deletion of element in another map. #99

Closed lucasvanbramer closed 2 years ago

lucasvanbramer commented 2 years ago

Describe the bug With two clients connected over a broadcastChannel to the same Doc, client1 deleting an element of map1 causes a value in map2 to be deleted on client1's side, but only after the client2 modifies the value of map2. Client1 and client2 then split and their documents do not share state. Client2 will behave normally on its side and client1 has inconsistent behavior depending on what happened in its version of map2.

To Reproduce This issue is pretty hard to reproduce without having a GUI to emit events manually from each client at the right time. Here is the simplest possible code to illustrate the phenomenon, with comments describing what happens step by step coming from which client:

const example = () =>
{
  // time t=-1:
  // SETUP: indicates structure. 
  // Does not matter who does this setup.
  // as whoever joins the broadcastChannel second 
  // gets the initial state from whoever is first.
  // I do not think this is important, but the level
  // of nesting may be what is causing this?
  const yDoc = new Y.Doc();
  const setupMapDepth0 = yDoc.getMap("setupMapDepth0");
  const setupMapDepth1 = new Y.Map();
  setupMapDepth0.set("setupMapDepth1", setupMapDepth1);
  const setupMapDepth2 = new Y.Map();
  setupMapDepth1.set("setupMap2", setupMapDepth2);

  const map1 = new Y.Map();
  setupMapDepth2.set("map1", map1);
  const map2 = new Y.Map();
  setupMapDepth2.set("map2", map2);

  const itemOne = new Y.Map();
  itemOne.set("text", "text1");
  map1.set("itemOne", itemOne);
  map2.set("counter", 0);

  // time t=0
  // CLIENT ONE does this
  // This is the critical line. 
  // Things only go wrong after a delete happens.
  map1.delete("itemOne");

  // time t=1: 
  // CLIENT TWO does this
  map2.set("counter", 1);

  // time t=2:
  // CLIENT ONE receives a change to counter: {action: 'delete', oldValue: 0}
  // CLIENT TWO receives a change to counter: {action: 'update', oldValue: 0}

  // time t=3:
  // After this point, changes are still successfully sent across the broadcastChannel. 
  // I was able to step through with the debugger but was not able to see any major
  // differences between behavior of readUpdatesV2 on client1 and client2.
  // However, the two clients' states are completely disconnected and remote changes
  // no longer propagate into the document, only local changes do.
};

Expected behavior At time t=2, we expect both clients one and two to receive the change to count {action: 'update', oldValue: 0}.

Screenshots None, but would be more than happy to share this happening live.

Environment Information

Additional context This is happening deep within my app, but again would be happy to share any further details because this has been kind of a blocker (I can work around not having delete functionality, but it would be hacky).

dmonad commented 2 years ago

Hi @lucasvanbramer,

Can you please log map._item.id in both browser windows and tell me if they are the same? Here, map is the Yjs type that doesn't sync anymore.

lucasvanbramer commented 2 years ago

They are the same and clock values are the same, even after t=2. (After t=2, the client who received the 'delete' on map2 is sometimes still receiving events for map1).

dmonad commented 2 years ago

Can you please check the value of map._item.deleted? - for both browser windows

lucasvanbramer commented 2 years ago

For both map1 and map2 (I am observing on both), they both indicate that map1._item.deleted and map2._item.deleted are false.

Perhaps an interesting detail - after the delete of counter on client1, client1's observer of map2 (the counter map) no longer emits events to .observe - I can only see that map2 is not deleted because the observer on map1 is still firing.

dmonad commented 2 years ago

You'll probably notice that doc.store.pendingStructs is not null, correct?

The issue that you described can happen when you manipulate the document in Yjs' events (e.g. change a value in the observer-call). This will cause the provider to send an update via broadcastchannel. However, the event will not be sent because I'm still using lib0/mutex to prevent users from propagating updates that they already received.

If you checked that pendingStructs is not null, then I'll try to find a solution for this (low priority). On your end, you can fix the issue by not manipulating the document within observer-calls. While I can fix some issues that result from doing this, it is generally discouraged to manipulate the document within the observer-call because doing it also screws up the order of events in some cases.

lucasvanbramer commented 2 years ago

You're right - I did notice pendingStructs was not null, and your explanation makes perfect sense. This is a fantastic, clear explanation and was exactly what I was looking for. Thank you so much for this assistance and all the work you've put into Yjs!

dmonad commented 2 years ago

@lucasvanbramer I fixed the issue in y-websocket@1.4.1

lucasvanbramer commented 2 years ago

Thank you!

hesselbom commented 2 years ago

@dmonad This fix unfortunately caused a regression bug wherein my project that uses yjs, y-codemirror and y-websocket suddenly started logging the following for every change/keystroke in CodeMirror:

[yjs] Changed the client-id because another client seems to be using it.

And it looks like every keystroke creates a new client id which then messes up a bunch of other things (e.g. remote cursors, etc.).

I'll try to create a repro when I have the time but just wanted to mention it here so it isn't lost.

For now I'll continue using y-websocket@1.4.0 which still works fine.