make the channel response table thread safe
HaraldWikeroy opened this issue · 3 comments
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<Phoenix.ChannelSubscription>>;
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:
@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.
After this change, two of the unit tests fail, btw.
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:
- There might be an unnecessary performance impact for Unity developers that are using a library that reliably runs callbacks on the original thread.
- 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!