Ghost Websocket Connections! about liveblocks HOT 4 CLOSED

Thanks for the kind words. Believe it or not, but I've run into this very issue two days ago myself, and I have been meaning to investigate it more deeply. I'll try to take a look at it tomorrow, because you're right, and this replication is amazingly helpful! 💯

from liveblocks.

Hey @dev-badace, I've opened a PR to fix this in #1463.

the ideal place to cleanup would be when machine exists the @connecting.busy. so right before the state is marked stale

Exactly. In this case, however, the @connection.busy is implemented as a promise-based state (because it uses .onEnterAsync()), which triggers a promise on enter, and will either (1) advance to @ok.connected when it resolves, or (2) handle any failure case otherwise.

The way the machine is constructed guarantees that socket is only observable/set when in the @ok.* states. Other states simply don't know about a socket instance by design. That why a cleanup function in the "on exit" handler for @connection.busy is less useful and may falsely suggest that socket is/can be set there.

So my fix is most in line with the ideal scenario: either the @connection.busy promise returns a succesful socket instance and advances to @ok.* sub states, or… an error happens and no traces are left behind. This fix takes care of the "leaving no traces behind" part, which was obviously missing here indeed.

Again, thanks for thinking along. Let me know what you think!

from liveblocks.

This fix is now published with v1.9.8.

from liveblocks.

I'm glad I was helpful. 😄

this is an interesting one. I'd like to also share a few solutions that came to my mind,

1. taking a third cleanup function for stale success cases however this still can lead to >1 open websocket connections till the window it's cleared by the cleanup function

2. a different variation of the above solution where the success case gets called with another argument boolean isStale, and checking for that, (hmm... i don't like this one).

the ideal place to cleanup would be when machine exists the @connecting.busy. so right before the state is marked stale

      return () => {
        cancelled = true;
        cleanup(this.current.context) //something like this, here we close the socket in context `tempSocket`
      };

maybe additional piece of info in the context like tempSocket that gets passed down to the cleanup via context.

from liveblocks.