-
Notifications
You must be signed in to change notification settings - Fork 27
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
make the channel response table thread safe #26
Comments
@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:
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! |
Thank you @Mazyod for this library. For the lurkers looking for answers, I was able to get a 24k channels Dict, not broadcasting very frequently but when it did it was at a burst of 24k messages. My use case is a unidirectional communication from a C# server with this library to a Phoenix app. This is not Unity at all. However to achieve this I had to do the suggested patch by @HaraldWikeroy in addition to patching the initial size of the channel List in the Socket.cs file to fit my tests and potentially breaking a few things in Channel.cs. My comment is purely informative and I have no additional input. Thanks again @Mazyod for creating this. |
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:
The text was updated successfully, but these errors were encountered: