Mazyod / PhoenixSharp

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

make the channel response table thread safe #26

Open HaraldWikeroy opened 1 year ago

HaraldWikeroy commented 1 year ago

Occasionally, I would get exceptions when assigning new events to the _bindings dictionary in Channel.cs . With the attached code, the error would appear after approx. 1000 pushes.

These exceptions happen because the socket replies are received on another thread. The _bindings dictionary is not thread safe so adds and removes crash.

So, in Channel.cs, change the type of SubscriptionTable to ConcurrentDictionary: using SubscriptionTable = System.Collections.Concurrent.ConcurrentDictionary<string, System.Collections.Generic.List>;

and use TryRemove on _bindings: public bool Off(string anyEvent) => bindings.TryRemove(anyEvent, out );

I've tested the updated code with tens of thousands of pushes and it doesn't crash now:

Snag_173ceda8

Mazyod commented 1 year ago

@HaraldWikeroy Thanks for raising this, and even suggesting a fix! I will get to it soon to review the issue and the fix.

Much appreciated.

HaraldWikeroy commented 1 year ago

After this change, two of the unit tests fail, btw.

Mazyod commented 1 year ago

Thanks again, @HaraldWikeroy, I am able to reproduce the problem using the approach you provided. However, I am a bit reluctant to simply change the data structure to a concurrent one for 2 reasons:

  1. There might be an unnecessary performance impact for Unity developers that are using a library that reliably runs callbacks on the original thread.
  2. More importantly, I am concerned of other possible places the code might fail due to unthread-safe conditions (Presence maybe).

I'm thinking of implementing a way where there is a "pluggable" adapter that synchronizes callbacks from the socket implementation using a queue or something. Then, we can recommend using it for certain websocket implementations, and not others. It can come by default with the WebsocketSharp implementation, for example.

Any additional feedback is more than welcome!