Skip to content
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

Open
HaraldWikeroy opened this issue Dec 20, 2022 · 3 comments
Open

make the channel response table thread safe #26

HaraldWikeroy opened this issue Dec 20, 2022 · 3 comments
Labels

Comments

@HaraldWikeroy
Copy link

HaraldWikeroy commented Dec 20, 2022

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:

Snag_173ceda8

@Mazyod
Copy link
Owner

Mazyod commented Dec 21, 2022

@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
Copy link
Author

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

@Mazyod Mazyod added the is:bug label Dec 24, 2022
@Mazyod
Copy link
Owner

Mazyod commented Dec 24, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants