Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mifopen opened this issue Oct 5, 2022 · 10 comments
Open

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

mifopen opened this issue Oct 5, 2022 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@mifopen
Copy link

mifopen commented Oct 5, 2022

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 mifopen added the bug Something isn't working label Oct 5, 2022
@mifopen
Copy link
Author

mifopen commented Oct 11, 2022

@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
Copy link
Member

dmonad commented Oct 12, 2022

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

@mifopen
Copy link
Author

mifopen commented Jan 25, 2023

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

@mifopen
Copy link
Author

mifopen commented Feb 20, 2023

@dmonad could you please give any comment on this?

@jamespantalones
Copy link

jamespantalones commented Jul 6, 2023

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

@jamespantalones
Copy link

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
Copy link
Author

mifopen commented Jul 7, 2023

@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
Copy link

jamespantalones commented Jul 7, 2023

@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
Copy link
Author

mifopen commented Jul 10, 2023

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
Copy link
Author

mifopen commented Jul 10, 2023

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants