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
fix: getYDoc should await bindState, if provided #39
Conversation
bin/utils.js
Outdated
if (doc.whenSynced) { | ||
await doc.whenSynced | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: If I move this await below the message listeners the original problem persists
Aye, I've run into this too, trying to implement a |
These changes are published on my fork, you're welcome to run it in the meantime |
Nice. |
OK think I found the other race condition. Putting the await where you did (before setting the
|
Makes sense in theory but it's not a race condition I've seen in practice – maybe I just made it more rare eh? |
It happens 100% of the time for me using your branch. All that's required is that 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. |
async bindState small changes
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 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)
})
}
}) |
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. |
That makes sense. I think what matters is that there should be no delay between when |
Hi, I haven't ignored this PR. I'm currently working on a refactor of y-websocket to use differential-updates yjs/yjs#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). |
@dmonad Nice. Lemme know if you'd like to collab on the API for custom persistence (in my case, postgres). |
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. |
If the websocket disconnects before bindState has applied the persisted value from the database then writeState will end up writing an empty ydoc
@wmhilton I found another race condition here that you should be aware of, see previous commit ^ |
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? |
Implemented these changes on my working branch at https://github.com/canadaduane/y-websocket/tree/async-bind-state for anyone who is interested. |
Closed as there's clearly no intention of merging this 😆 |
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.Huly®: YJS-750