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

Editor content never syncs when connection is not immediately handed over to hocuspocus using authentication middleware #122

Closed
jggc opened this issue Jun 17, 2021 · 8 comments
Labels
bug Something isn't working
Projects

Comments

@jggc
Copy link
Collaborator

jggc commented Jun 17, 2021

Description
Somewhere in the y-websocket/hocuspocus client/server stack (sorry for not knowing exactly where it comes from) :

When a connection is established with hocuspocus and reset quickly after the instantiation from the client the document's data is not sent properly to the client.

Steps to reproduce the bug see #122 (comment) , more accurate after investigation
hocuspocus + rocksdb server
tiptap + y-websocket frontend

let provider,
  ydoc = new Y.Doc(),
  docId = 'somedoc';
const registerProvider = () => {
  provider = new WebsocketProvider('ws://somehost', docId, ydoc);
};

const resetConnection = () => {
  provider.destroy();
  registerProvider();
};
// <===== HERE, resetting the provider on the document 
// before the document is properly fed from the backend prevents it to ever get its full data
// try with different values between 1 and 100 and you'll reproduce
setTimeout(resetConnection, 10); 

new Editor({
  extensions: [
    StarterKit,
    SomeCustomExtension,
    Collaboration.configure({ document: ydoc }),
   ]
});

Expected behavior
Should initialize editor content properly even when connection is resetting a few times quickly

Additional context
I am using firebase authentication to authenticate the user before handing the connection over to hocuspocus, but firebase authentication tokens can change often. So, if the client disconnects and tries to reconnect using y-websocket internal reconnection logic the token may be expired and the server will reject the connection. To avoid that I reset the connection on every onIdTokenChanged , and one of those happen within the first second of a page being loaded.

So I can work around that by preventing the connection reset within the first few seconds, or even better by allowing y-websocket query params to be functions that are called every time a connection attempt is made.

@jggc jggc added the bug Something isn't working label Jun 17, 2021
@jggc
Copy link
Collaborator Author

jggc commented Jun 17, 2021

I kept investigating and I think my assumption above is wrong, it seems to be a timing issue caused by my authentication on the server happening before handing over the connection to hocuspocus and after the websocket connection is established.

So there still is a bug but the repro steps may be different from the above.

@jggc
Copy link
Collaborator Author

jggc commented Jun 17, 2021

Nailed it down : when the websocket connection is handed over to hocuspocus not quickly enough (more than about 10 millis), hocuspocus (or y-websocket server?) will miss the first message(s) sent by the client and will never properly sync.

So a way I can 100% repro is :

app.ws('/:document', (websocket: ws, request: Request) => {
  setTimeout(() => {
    hocuspocus.handleConnection(websocket, request, request.params.document)
  }, 1000); // <=== removing or reducing the timeout to below 10 ms fixes it every time and a timeout of 1s causes the bug every time
})

The fact that the first message is missed causes the client to never properly sync. The client side websocketProvider.on('sync', ...) event is not fired. Sometimes the document data is still sent properly but its really not reliable.

@hanspagel sorry for my initial report not being 100% accurate it was a tricky one for me who doesn't know Y.js or CRDTs at all.

@jggc jggc changed the title Editor content not initialized properly when connection resets quickly Editor content never syncs when connection is not immediately handed over to hocuspocus using authentication middleware Jun 17, 2021
@hanspagel
Copy link
Contributor

hanspagel commented Jun 17, 2021

No worries, it's a great bug report. One question though: Are you on the latest @hocuspocus/server version? I thought this is fixed.

@jggc
Copy link
Collaborator Author

jggc commented Jun 17, 2021

You made me doubt for a second since rocksdb extension is alpha.59 and my server is alpha.58 but from npmjs it really seems to be latest and I checked my source of truth :

cat node_modules/@hocuspocus/server/package.json 
{
  "name": "@hocuspocus/server",
  "description": "plug & play collaboration backend",
  "version": "1.0.0-alpha.58",

@tommoor
Copy link
Sponsor Collaborator

tommoor commented Aug 11, 2021

when the websocket connection is handed over to hocuspocus not quickly enough (more than about 10 millis), hocuspocus (or y-websocket server?) will miss the first message(s) sent by the client

This kind of makes sense to me, if there's a time between setting up the connection and letting hocuspocus know about it then it seems like it would be upto the surrounding application to capture and enqueue messages that come in that time? It might mean that handleConnection would need an optional parameter to accept messages that were queued.

@hanspagel
Copy link
Contributor

hanspagel commented Aug 30, 2021

Makes sense to me, too! But the question for me is: Why should there be time between setting up the connection and letting hocuspocus know about it?

If there is a valid use case, I can add a parameter to pass queued messages. ✌️

You’re using Express, right? You’d probably need to set up the queuing yourself then though.

@hanspagel hanspagel added this to To do in hocuspocus via automation Aug 30, 2021
@hanspagel hanspagel moved this from To do to In progress in hocuspocus Aug 30, 2021
@jggc
Copy link
Collaborator Author

jggc commented Sep 15, 2021

It's been a little while since I've messed with this but my use case was to handle authentication before involving hocuspocus at all. Since I have to validate authentication against a remote provider it takes time and would mess up the message order.

My first idea when I opened this bug is that hocuspocus server should send some sort of hocuspocus is ready event to signal the client that he is handling the connection.

My thought process behind this is :

  • It's the client's responsibility to request the http(s)+ws connection whenever he is ready to do so
  • It's the server's responsibility to tell the client when he is ready to handle commands over the websocket after he is fully satisfied with the client's request and has done all required validations if any
  • At this point the bi-directional handshake is complete and they both are ready to exchange further messages

That said, I worked around it by using a hook (onConnect IIRC) and it's fine this way, I'm honestly not sure if there is a valid use case for messing with the connection before handing it over but I still think that it would make sense to have the server explicitly tell the client that he's ready when the connection is handed over.

Either way I'm fine with or without this feature so feel free to close if you feel it's not relevant enough.

@hanspagel
Copy link
Contributor

Thanks for getting back! I see your point, but yes, I think it’s better to handle the authentication in hocuspocus.

I’m closing this as I don’t think I’ll work on this soonish. But for everyone reading this: Feel free to comment, if there’s interest I’ll deal with it.

Thanks again for the report @jggc!

hocuspocus automation moved this from In progress to Done Sep 15, 2021
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
No open projects
Development

No branches or pull requests

3 participants