fix: getYDoc should await bindState, if provided

[tommoor]

If bindState is not awaited then the doc is indeterminately updated from persistence state, this can result in sync step 2 being sent to the client before the content of the doc had been updated from the persistence store.

[billiegoose]

Aye, I've run into this too, trying to implement a postgres backend. I be needing a way to await an asynchronous bindState. How can I help mateys?

[tommoor]

These changes are published on my fork, you're welcome to run it in the meantime

[billiegoose]

Nice. ~Good thing he hasn't merged it yet though, it still triggers a race condition for me. Lemme see if I can open a PR on your PR...~ Edit: IDK what's wrong now, I think I need sleep 😴 😆

[billiegoose]

OK think I found the other race condition. Putting the await where you did (before setting the conn.on('message' handler) causes my unit test to hang, I think because any messages sent before the event handler is set are simply dropped. What is working for me is to await whenSynced in two places:

1. in setupWSConnection right before // send sync step 1
2. in messageListener right after case messageSync:

[tommoor]

Makes sense in theory but it's not a race condition I've seen in practice – maybe I just made it more rare eh?

[billiegoose]

It happens 100% of the time for me using your branch. All that's required is that await bindState be slow compared to the websocket connection.

IIUC both the client and the server send "sync step 1" messages to each other. So it boils down to who sends / receives first. Essentially there's two cases: the server sends sync step 1 first, or the server receives sync step 1 first. I suggest awaiting in both situations.

[billiegoose]

@tommoor Should I make a PR to your branch so you can battle-test the proposed change?

Update: I made a PR to your branch. https://github.com/tommoor/y-websocket/pull/1

[billiegoose]

I continued to have issues even after this change, and narrowed it down to the fact that the example code for adding auth,

server.on('upgrade', (request, socket, head) => {
  // You may check auth of request here..
  /**
   * @param {any} ws
   */
  const handleAuth = ws => {
    wss.emit('connection', ws, request)
  }
  wss.handleUpgrade(request, socket, head, handleAuth)
})

is incorrect. Done this way, it accepts the WebSocket connection immediately, and messages from the client get dropped while authentication is happening because setupWSConnection hasn't been called yet. It also results in odd errors on the client in the Network panel for authorization rejection because one ends up sending a 401 status after the WS connection has been "successfully" upgraded.

The correct usage is:

server.on('upgrade', (request, socket, head) => {
  // You may check auth of request here..
  const unauthorized = false;
  if (unauthorized) {
    socket.write('HTTP/1.1 401 Unauthorized\r\n\r\n');
    socket.destroy();
  } else {
    wss.handleUpgrade(request, socket, head, ws => {
      wss.emit('connection', ws, request)
    })
  }
})

[tommoor]

I think there are a number of ways to do this, our server is setup differently and performs the authentication after the websocket upgrade with the first message over the socket. Glad you figured it out – the other changes here have been solid for us for a couple of weeks now.

[billiegoose]

That makes sense. I think what matters is that there should be no delay between when handleUpgrade is called and when setupWSConnection is called. Because during that time, the client can send messages but the server hasn't added a message handler callback. Either doing auth after setupWSConnection, or before handleUpgrade should work.

[dmonad]

Hi, I haven't ignored this PR.

I'm currently working on a refactor of y-websocket to use differential-updates https://github.com/yjs/yjs/issues/263 . I want to enable the server to sync with the client without loading the Yjs document at all.

Thanks for sharing this PR @tommoor , I want to publish this change with the refactor to reduce the friction (I didn't have time to test whether this leads to other issues).

[billiegoose]

I want to enable the server to sync with the client without loading the Yjs document at all.

@dmonad Nice. Lemme know if you'd like to collab on the API for custom persistence (in my case, postgres).

[dmonad]

Sure thing. I'll create a PR that you can review and see if the approach will still work for you. I want to stay compatible as much as possible. But it might require you to slightly adapt your implementation.

[tommoor]

@wmhilton I found another race condition here that you should be aware of, see previous commit ^

[canadaduane]

I just realized this explains a bug I've been seeing where exporting documents via API only work on the 2nd attempt (since the first attempt "warms" it into memory, and the 2nd attempt succeeds). Is there any thought to mainlining these changes, or perhaps to integrating the differential-updates work?

[canadaduane]

Implemented these changes on my working branch at https://github.com/canadaduane/y-websocket/tree/async-bind-state for anyone who is interested.

[tommoor]

Closed as there's clearly no intention of merging this 😆