yjs / y-websocket

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

Reconnected client becomes visible to others only after some period #122

Open mifopen opened 1 year ago

mifopen commented 1 year ago

Let's say we have two clients: 1 and 2 that synchronise their awareness states via some server node. They both have some states. Now if we call provider.disconnect() on the second client then the first one will notice it immediately. But if we call provider.connect() then no event will raise on the awareness instance of the first one. added, updated, removed arrays will be empty because of the following condition:

    if (currClock < clock || (currClock === clock && state === null && awareness.states.has(clientID))) {

In our case currClock === clock but state is not null and there is no clientID in states map because client was just removed from it after disconnecting. The problem as I can see it is that clock stays the same. I applied the following workaround:

        provider.on('status', ({ status }) => {
            if (status === 'connected') {
                provider.awareness.setLocalState(provider.awareness.getLocalState());
            }
        });

just to tick the clock but I think it makes sense to implement it inside y-websocket library

mifopen commented 1 year ago

@dmonad Also another issue is that on reconnection we can receive the current awareness state of others (e.g. https://github.com/yjs/y-websocket/blob/master/bin/utils.js#L278-L284). But their clocks could be still the same as we already have in meta because we're not purging meta on reconnection. As a result, we wouldn't handle initial awareness state from others until the next clocks tick.

dmonad commented 1 year ago

That's a good point. Thank you, I'll look into this!

mifopen commented 1 year ago

Hey! Is there any way we can help with it?

mifopen commented 1 year ago

@dmonad could you please give any comment on this?

jamespantalones commented 1 year ago

@mifopen did you ever get this sorted? running into same issue

jamespantalones commented 1 year ago

also, @dmonad, the line in question in the y-protocols/awareness file:

if (currClock < clock || (currClock === clock && state === null && awareness.states.has(clientID))) {...}

is also blocking us after a reconnection. state is not null, but awareness.states does not contain the clientID. do you have any other workaround?

I'm unable to get any updates through until the outdatedTimeout kicks in

mifopen commented 1 year ago

@mifopen did you ever get this sorted? running into same issue

I mixed two issues into one, so let me explain our workarounds one by one.

The one described in the starter post we are still fixing with

provider.on('status', ({ status }) => {
  if (status === 'connected') {
    provider.awareness.setLocalState(provider.awareness.getLocalState());
  }
});

So just an artificial clock ticking.

Next regarding the one I added as a comment: We've made some changes to y-protocols/awareness.js implementation and besides added, updated, removed also introduced resurrected. So that we can react on someone reconnected immediately. It turned out to be useful to have it as a separate array as you don't always want to know about that fact somehow. At least we found such cases. I can share an implementation with you if you want, it's ~4 lines of changes to awareness.js

jamespantalones commented 1 year ago

@mifopen thanks! would love to see your implementation. for some reason, i am unable to get the clock to tick via settting setLocalState... provider.awareness.getLocalState() is an empty object at the time of status === 'connected'

mifopen commented 1 year ago

Here's the patch to introduce resurrected nodes

diff --git a/node_modules/y-protocols/awareness.js b/node_modules/y-protocols/awareness.js
index 8f9ae94..b4a4603 100644
--- a/node_modules/y-protocols/awareness.js
+++ b/node_modules/y-protocols/awareness.js
@@ -10,7 +10,7 @@ import { Observable } from 'lib0/observable'
 import * as f from 'lib0/function'
 import * as Y from 'yjs' // eslint-disable-line

-export const outdatedTimeout = 30000
+export const outdatedTimeout = 70000

 /**
  * @typedef {Object} MetaClientState
@@ -245,6 +245,7 @@ export const applyAwarenessUpdate = (awareness, update, origin) => {
   const updated = []
   const filteredUpdated = []
   const removed = []
+  const resurrected = []
   const len = decoding.readVarUint(decoder)
   for (let i = 0; i < len; i++) {
     const clientID = decoding.readVarUint(decoder)
@@ -275,7 +276,10 @@ export const applyAwarenessUpdate = (awareness, update, origin) => {
       } else if (clientMeta !== undefined && state === null) {
         removed.push(clientID)
       } else if (state !== null) {
-        if (!f.equalityDeep(state, prevState)) {
+          if (clientMeta !== undefined && prevState === undefined) {
+              resurrected.push(clientID)
+          }
+          if (!f.equalityDeep(state, prevState)) {
           filteredUpdated.push(clientID)
         }
         updated.push(clientID)
@@ -284,12 +288,12 @@ export const applyAwarenessUpdate = (awareness, update, origin) => {
   }
   if (added.length > 0 || filteredUpdated.length > 0 || removed.length > 0) {
     awareness.emit('change', [{
-      added, updated: filteredUpdated, removed
+      added, updated: filteredUpdated, removed, resurrected
     }, origin])
   }
   if (added.length > 0 || updated.length > 0 || removed.length > 0) {
     awareness.emit('update', [{
-      added, updated, removed
+      added, updated, removed, resurrected
     }, origin])
   }
 }
diff --git a/node_modules/y-protocols/dist/awareness.cjs b/node_modules/y-protocols/dist/awareness.cjs
index e52c5d3..f1e649c 100644
--- a/node_modules/y-protocols/dist/awareness.cjs
+++ b/node_modules/y-protocols/dist/awareness.cjs
@@ -40,7 +40,7 @@ var f__namespace = /*#__PURE__*/_interopNamespace(f);
  * @module awareness-protocol
  */

-const outdatedTimeout = 30000;
+const outdatedTimeout = 70000;

 /**
  * @typedef {Object} MetaClientState
@@ -275,6 +275,7 @@ const applyAwarenessUpdate = (awareness, update, origin) => {
   const updated = [];
   const filteredUpdated = [];
   const removed = [];
+  const resurrected = [];
   const len = decoding__namespace.readVarUint(decoder);
   for (let i = 0; i < len; i++) {
     const clientID = decoding__namespace.readVarUint(decoder);
@@ -305,6 +306,9 @@ const applyAwarenessUpdate = (awareness, update, origin) => {
       } else if (clientMeta !== undefined && state === null) {
         removed.push(clientID);
       } else if (state !== null) {
+        if (clientMeta !== undefined && prevState === undefined) {
+          resurrected.push(clientID)
+        }
         if (!f__namespace.equalityDeep(state, prevState)) {
           filteredUpdated.push(clientID);
         }
@@ -314,12 +318,12 @@ const applyAwarenessUpdate = (awareness, update, origin) => {
   }
   if (added.length > 0 || filteredUpdated.length > 0 || removed.length > 0) {
     awareness.emit('change', [{
-      added, updated: filteredUpdated, removed
+      added, updated: filteredUpdated, removed, resurrected
     }, origin]);
   }
   if (added.length > 0 || updated.length > 0 || removed.length > 0) {
     awareness.emit('update', [{
-      added, updated, removed
+      added, updated, removed, resurrected
     }, origin]);
   }
 };
mifopen commented 1 year ago

Regarding empty getState — we don't use awareness instance as a "primary" storage for the current presence information. We store it in some variable separately and sync with awareness instance every N ms (we're tracking cursor movements and have to throttle the frequency of updates sent to other nodes)