Mazyod / PhoenixSharp

C# Phoenix Channels client. Unity Compatible.
MIT License
163 stars 27 forks source link

"phx_join" is never sent if socket is not open by the time `Join` is called #19

Closed arne-schroppe closed 2 years ago

arne-schroppe commented 2 years ago

Creating a channel and immediately joining it will cause the "phx_join" message to be stuck in Channel's sendBuffer forever, if the underlying WebSocket is not open by the time Join is called.

Example code:

var fooChannel = _socket.MakeChannel("foo");
fooChannel.Join(new Dictionary<string, object>(), _timeout)

The second line will eventually call Socket.Push(Message). That method will check if the channel's socket is already in state Open. If it isn't, it will return false which will cause its calling method, Channel.Push(Push, TimeSpan?), to add the "phx_join" message to its sendBuffer. But FlushSendBuffer is only called in OnJoinReply. So it can't ever be sent and the channel will never be joined.

Mazyod commented 2 years ago

Hello @arne-schroppe , this sounds like a serious issue. Thanks for the report. I will try to look into it asap. If anyone is interested in opening a PR with test coverage, that would also be fine by me 👍

arne-schroppe commented 2 years ago

Sounds good! I can also add a bit more context: Our original implementation of IWebsocket was based on websocket-sharp, where we never ran into this issue.

We only started noticing this bug when we built the project for WebGL. For that we had to use native Javascript WebSockets and they seem to take longer to actually open whereas it looks to me (didn't fully verify) that websocket-sharp switches to the "open" state immediately: WebSocket.cs#L1282. So it's possible that most people never run into this issue because they use a WebSocket that behaves similar to websocket-sharp

Mazyod commented 2 years ago

I haven't revisited the reference Phoenix client implementation in a while... but it seems the sendBuffer is now the responsibility of the socket object, which makes more sense.

I'll look into revising the implementation and updating it to be consistent with the reference implementation from the official phoenix framework repository.

Once that's done, I'll add a test case with your scenario and hope it will simply pass :)

https://github.com/phoenixframework/phoenix/blob/master/assets/js/phoenix/socket.js#L94

Mazyod commented 2 years ago

@arne-schroppe I've rewritten the library to be based on Phoenix JS. The unit tests now pass for the case you mentioned. It would be very helpful if you would try it out and give your valuable feedback: #20

I still plan to go over the rewrite, comparing it with the previous version, do some tweaks, test it on my app, update the docs, ... etc.

arne-schroppe commented 2 years ago

@Mazyod I'll have a look and write my feedback in the PR. And thanks for spending time on fixing this!