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

WebSocket Ready flag should only be set after Open event #297

Open
justin0mcateer opened this issue Feb 28, 2024 · 2 comments
Open

WebSocket Ready flag should only be set after Open event #297

justin0mcateer opened this issue Feb 28, 2024 · 2 comments

Comments

@justin0mcateer
Copy link

Currently, the WebSocket Client Transport will mark itself as 'ready' once it has received a PeerMessage from the server, otherwise one second after construction.

However, if for some reason the WebSocket takes more than one second to fully establish, the Transport will be marked ready and will emit the 'ready' event prior to the WebSocket transport actually being ready to accept 'send' calls. This could be resolved by moving the 'auto-ready' setTimeout call into the 'onOpen' callback function.

We are periodically seeing 'Websocket not ready' messages emitted by this line:
https://github.com/automerge/automerge-repo/blob/main/packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts#L141

Since Automerge Repo is fully responsible for sending messages, either there is an issue with the 'ready' handling, or the Transport is being marked 'ready' before the WebSocket is in the OPEN state. It should be noted that we were seeing this issue with the public/test Websocket server, which may be slower/more latent.

@pvh
Copy link
Member

pvh commented Feb 28, 2024

Thanks for the bug report, @justin0mcateer! I see the comment indicates that this choice was deliberate by @alexjg so before I change this let's see if he has any comment on why he set it up that way.

@alexjg
Copy link
Contributor

alexjg commented Feb 29, 2024

The reason for the setTimeout which marks the adpater as ready is because we don't do anything until all the network adapters have said they are ready. The idea of the "ready" callback was not to wait for peers who might never appear (because for example they are overloaded and will never respond), but just to avoid situations where an adapter will take a few miliseconds to set up (such as for the MessageChannel adapter) and consequently races with a Repo.find call on application startup. Frankly, this is a bit of a hack, a more principled approach probably means introducing some additional states in the NetworkSubsystem to track the state of an adapter (connecting, handshaking, ready etc.).

In this case however, we can probably continue hacking. The problem is that the setTimeout is in the connect handler, which means that if we take longer than a second to connect then we end up telling the network adapter we are ready for messages when we aren't. Probably the easiest way around this is to make the readiness timeout configurable - with the option to have no timeout whatsoever. It should be noted though that until we mark the adapter as ready the Repo will not do any networking at all, which means you'll force all requests for a document to wait on the adapter being ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants