doghappy / socket.io-client-csharp

socket.io-client implemention for .NET
MIT License
715 stars 124 forks source link

Canceling the ConnectAsync() with a CancellationToken #377

Open Aspher0 opened 1 month ago

Aspher0 commented 1 month ago

Hello dog 😄

First of all, thanks for the project, it helped me a lot. However, I'm having struggle to cancel an ongoing connection attempt initiated with ConnectAsync, would it be possible to maybe add an overload to the ConnectAsync() method with a CancellationToken parameter so we can cancel it anytime please ? I tried forking the project but I ended up breaking everything, I'm sadly not that talented ahah

I tried disposing the client in both 3.1.1 version and 3.0.8 as someone had suggested in the issues, but no luck, it will keep trying to connect even after disposing.

Thanks in advance for your answer!

doghappy commented 1 month ago

Hi @Aspher0, thanks for your feedback. make a new overload to let user pass a CancellationToken is a good idea. but currently we have an option: ConnectionTimeout, the lib will create CancellationToken based on ConnectionTimeout.

What will happen if we have the following code?

using var io = new SocketIO(new SocketIOOptions
{
    ConnectionTimeout = TimeSpan.FromSeconds(1)
});

using var cts = new CancellationTokenSource();
await io.ConnectAsync(cts.Token);
...
cts.Cancel();

Questions:

  1. I just want to say if we added that overload, we will have 2 CancellationTokens, finally which 1 should be used? any suggestion?
  2. could you explain your scenario, why you need to Cancel manually?
Aspher0 commented 1 month ago

Hello there, thanks for your answer and interest.

First of all, if you want context, I use your library in my project named "GlamMaster" which can be found here, in SocketManager.cs and SocketOnEventsManager.cs : https://github.com/Aspher0/GlamMaster/blob/main/GlamMaster/Socket/

So, I do have a ConnectionTimeout already set, but it doesn't fixes anything. Let me explain. So, basically, my use case is that my program can have a socket connected or not, and I want the user to be able to abort the connection at any time, without having to wait for the connection timeout to end. Since there is no way to specify a cancellation token to manually cancel the socket connection, I thought disposing the client would prevent the socket from firing events such as the OnError, OnReconnectAttempt and so on. But it turns out that those events are still fired even after being unsubscribed, the client disposed and everything else. I actually had to use some weird shenanigans to make the client not do unexpected behavior, and since I have to actually dispose the client to "cancel it" (or somewhat cancelling it), I have to make a new client everytime I want to make a new connection. Now, let's say my socket server is down. My users would click the "connect button" and then, seeing it doesnt work, they would click the abort button and try to connect again, maybe more than once. If the server gets back online somehow, multiple clients would be connected at the same time on the server, and multiple clients would be referenced in the program's memory and would probably cause increased memory usage and so on.

I did end up finding a way to avoid every problems, but still, the events still being fired even after being unsubscribe and the client disposed, as well as the other issues I mentionned, makes my solution very messy and dirty.

Now, let's say you make an overload of the ConnectAsync function, you could let the user store their cancellationtoken on their side (you wouldn't need to worry about taking care of it, the user has to take care of that) and then you would just pass the token in the overloaded function and all of its tasks and stuff ? Then, in your exemple, if the cts.Cancel() is called, the connection would stop and everything would be cancelled and the client could catch the cancellation exception.

Basically :

try
{
    await io.ConnectAsync(cts.Token);
}
catch (OperationCanceledException e)
{
    // The socket connection has been aborted, do stuff
}
// Other catches here if needed
finally
{
    cts.Dispose();
}

I saw that you have a while loop that basically check if the token is cancelled or not, maybe have a boolean variable that specifies if it's a user-specified token or the automatically generated one.

Aspher0 commented 1 month ago

Hello @doghappy Any news ? 😄 Have a nice day!

doghappy commented 1 month ago

Hi, Sorry for the slow response.

My boss gave me too much work 🤣 and I have to take care of my baby👧, so I can only do maintenance no more than 8 hours a week.

Thanks for your explanation, by default the lib will always try to connect to the server when you calling ConnectAsync(), I'm not sure if setting Reconnection to false will solve your problem.


And here is a solution: https://stackoverflow.com/a/29623382/7771913 I will try the solution later.

Please leave a message if Reconnection = false is useful.

if (useful)
{
    Thread.Sleep(TimeSpan.FromDays(7));
}
DoItAsap();
Aspher0 commented 1 month ago

Hey @doghappy Thank you for your answer 😄 Don't worry, work is important and family is even more !

I haven't tried the Reconnection = false yet but I'd like to let my users choose the number of reconnection attempts and other options so I don't think it is a viable solution for me. When I have time, I will still try it and tell you if it works.

Also, the stackoverflow link you linked doesn't seem relevant in our case, I don't really see how it could help since your project does not let the user pass a cancellation token to the ConnectAsync() function.

If the solution does not work, I will retry forking your project again and add an overload of the ConnectAsync function and maybe open a pull request, if I can get it to work properly this time 😄

Take care !

doghappy commented 6 hours ago

I thought disposing the client would prevent the socket from firing events such as the OnError, OnReconnectAttempt and so on. But it turns out that those events are still fired even after being unsubscribed, the client disposed and everything else.

I checked the PR, it is a bit complex if we added another cancellation token.

If we improve Dispose(), after Dispose() was called

That means when you call the Dispose(), the instance looks like a new object.

As you said:

Now, let's say my socket server is down. My users would click the "connect button" and then, seeing it doesnt work, they would click the abort button and try to connect again, maybe more than once.

What do you think?

Aspher0 commented 4 hours ago

Hello @doghappy

You are right, and, in fact, I think we have 2 issues here. The first one being the fact that a user can not cancel ConnectAsync by themselves with a cancellationToken. The second one being the events such as OnError and so on being triggered even after the Dispose() is called.

I think we should implement the ConnectAsync overload which takes a CancellationToken parameter but also improve the Dispose() function so that no event is fired and nothing else unintended happens.

I don't think disposing the client completely is a good idea when you only want to cancel a connection. I might be wrong though, what do you think ?