pusher / pusher-websocket-dotnet

Pusher Channels Client Library for .NET
MIT License
111 stars 113 forks source link

Deadlock when unsibscribe from channels #111

Closed adamvas closed 3 years ago

adamvas commented 3 years ago

Hi, It can casue deadlock if you are unsubscribing from a channel in an async awaited method, like:

public override Task Shutdown()
{
    channel.UnbindAll();
    channel.Unsubscribe(); // <- wrapped **blocking** async call  : Task.WaitAll(_pusher.ChannelUnsubscribeAsync(Name)); 
    return pusherClient.DisconnectAsync();
}

eg:

public async Task DeactivatedAsync()
{
    ...
    await connector.Shutdown(); // <- async **non-blocking** call
    ...
}

Thats bcs of this:

 public void Unsubscribe()
{
    Task.WaitAll(_pusher.ChannelUnsubscribeAsync(Name)); 
}

Blocking async call in a non-blocking async call.

Best practice is usually async all-way-down.

erwin42 commented 3 years ago

I will look at fixing this. However you could use the following instead:

public override Task Shutdown()
{
    channel.UnbindAll();
    pusherClient.UnsubscribeAsync(channel.Name);
    return pusherClient.DisconnectAsync();
}

Please let me know if the above works?

adamvas commented 3 years ago

Hi @erwin42 , sure I changed my code to this already. It works as expected, just wanted to highlight it. Is there any reason behind that Unsubscribe is not async by default, I mean Task instead of void?

erwin42 commented 3 years ago

We would have removed the function, but we kept it for backwards compatibility. You will notice no mention of the Channel.Unsubscribe method in the Readme doc. We are trying to discourage people from using the method in favour of Pusher.UnsubscribeAsync.

I tried to repro your issue but couldn’t. I will leave a test in for it in our next release.

erwin42 commented 3 years ago

I will add public async Task UnsubscribeAsync() to the `Channel' class in our next release to help with this issue

elverkilde commented 3 years ago

hi @adamvas and thanks again for reporting an issue. The async method was added in #112 and released in version 2.1.0.