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

CancellationToken support #52

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Foxite
Copy link

@Foxite Foxite commented Aug 3, 2021

I've added support for passing CancellationTokens into RconSharp.

Rationale The main reason why I decided to do this, is because my Minecraft server shuts down automatically when nobody is online for 20 minutes, and starts automatically when some tries to join. The web app I wrote for managing the server doesn't play nice with this system, as the RconClient is used as a singleton within the app. If I try to execute a command after the server has shut down, even if the server has already started again, a reply never arrives because the server does not have an open connection with my app, even though RconSharp thinks it has an open connection with the server.

So I did this so you can specify a timeout for ExecuteCommandAsync and ConnectAsync. However, there is a weird problem with the compiler. If I try to compile this right now, there is an error in SocketChannel.cs, line 81, where the compiler can't see the extension method ConnectAsync(string, int, CancellationToken) even though it clearly exists within the same class as the ConnectAsync that was previously used. After some investigation, I found out that there are two distinct CancellationTokens: one defined in System.Runtime.dll, and the other in System.Private.CoreLib.dll. The first one is used by RconSharp, and the second one is used by SocketChannelExtensions. I'm not sure why this is a problem for ConnectAsync, because SendAsync and ReceiveAsync do not have this problem.

Directly calling the extension method as System.Net.Sockets.SocketTaskExtensions.ConnectAsync(socket, host, port, cancellationToken) does not work either.

I was wondering if you had any idea how to fix this.

(These changes were not tested because of this issue.)

@Foxite
Copy link
Author

Foxite commented Aug 3, 2021

Okay, it seems that the problem has nothing to do with System.Private.CoreLib but rather, SocketTaskExtensions does not contain an overload for ConnectAsync that takes a CancellationToken -- in .NET Standard. There is one in .NET Core; for some reason my IDE was decompiling the System.Net.Sockets assembly for .NET Core rather than Standard, which made me think that it was supported.

I'm not sure how to proceed from here as there seems to be no way to connect to a websocket while providing a CancellationToken in .NET Standard.

@stefanodriussi
Copy link
Owner

Looking at the docs, it seems like the overload for ConnectAsync that accepts a CancellationToken is only available in .NET 5 and .NET 6 Preview , although support for cancellation token is present for SendAsync method also in netstandard.
In general, I don't really like the idea of a connection open as a Singleton because unless you have some kind of keep alive, you never really know when it will go down.
I'll look into adding some kind of reconnection mechanism. What do you think ?

@Foxite
Copy link
Author

Foxite commented Aug 4, 2021

If it's possible to find out that the connection is lost, then that's probably a good idea.

I agree that a singleton connection isn't great. I originally wanted to put a limited lifetime on the client, as requests from the frontend tend to come in "pulses" (it refreshes 3 datasets every 5 seconds), but I wasn't able to find a good way to implement this as controllers in ASP.NET are instantiated for every request, as is every non-singleton dependency for those controllers. I guess I can manually implement a lifetime for the RconClient using a timer.

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

Successfully merging this pull request may close these issues.

2 participants