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

[@y-sweet/client] provider.disconnect() doesn't work #386

Open
dtkav opened this issue Jan 28, 2025 · 1 comment
Open

[@y-sweet/client] provider.disconnect() doesn't work #386

dtkav opened this issue Jan 28, 2025 · 1 comment

Comments

@dtkav
Copy link

dtkav commented Jan 28, 2025

provider.disconnect() is broken in the client because the handler for websocket.onclose calls connect() indiscriminately:
https://github.com/jamsocket/y-sweet/blob/44f4608004b35145d6f8885b87da0752e56f13d7/js-pkg/client/src/provider.ts#L585C1-L585C19

I had to call (provider as any).websocket.onclose = null before disconnect() in order to get it to work.

I think this is why separating intent (shouldConnect) from status is important.
shouldConnect is really not the same thing as status == 'offline' as is suggested by the deprecation notice.

Suggested fix is to bring back a settable shouldConnect property. Set this when the user calls disconnect() and then check it in the websocket.onclose and websocket.onerror handlers. Only reconnect if shouldConnect is true.

@paulgb
Copy link
Member

paulgb commented Feb 2, 2025

Thanks for catching this.

The disconnect function sets the status to STATUS_OFFLINE, but it seems that what's happening is that the onclose handler has already been called by the time we get there, so we enter this loop which we should not.

We could also guard against connect being called in the first place from the onclose handler if the status is offline, but it wouldn't solve the problem because it would also be called before the state update. I think switching the order they are called in the disconnect function should switch it.

I generally agree about separating state from intent, and if this were any more complex of a state machine I would, because treating intent as state tends to explode the set of possible states, but in this case it's essentially the same set of states -- if shouldConnect is false we should always be offline; if shouldConnect is true we are either connected or should be trying to be.

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

2 participants