Halt execution until server/client is ready after calling start
Closed this issue · 24 comments
Is there a way to block execution until the server is actually ready? Atm I'm doing a while loop that checks the IsListening
variable which seems wasteful. Same with the client, except checking if the client IsConnected
.
I would love to be able to do something like:
await WebsocketServer.Start()
await WebsocketClient.Start()
We could add a StartAsync
method to both, would that work? In the server it would return the AcceptConnections
task, and in the client, it would return the ConnectAsync
task; both could be awaited.
Hi @Alig96 I just pushed v2.2.1 to NuGet which adds a StartAsync
to both client and server. Mind giving it a try and letting me know if this is what you need?
NuGet: https://www.nuget.org/packages/WatsonWebsocket/2.2.1
Commit: f57cb56
Awesome, thanks for the quick update, I have got some good news and bad news;
- I was able to replace my wait loop for the client and works great
- I was not able to replace my wait loop for the server as the await from
await server.StartAsync()
never completes. I upped my test time out from 2 seconds to 30 and it still doesn't complete the await
Any suggestions?
Which OS and framework are you using? I just tested with the Test.Server and Test.Client project and it worked. Will need some help reproducing.
OS: MacOs Catalina (10.15.7)
Framework: net5.0
Here's a reproduction sample (Requires NUnit)
using System;
using System.Threading.Tasks;
using NUnit.Framework;
using WatsonWebsocket;
namespace WatsonServerHoldDemo
{
public class Tests
{
[Test]
[Timeout(5000)]
public async Task Test1()
{
var server = new WatsonWsServer();
await server.StartAsync();
Console.WriteLine("online :)");
}
}
}
"Online :)" is never hit :(
I can also attempt to try this on Ubuntu later tonight if you beleive this is an o/s issue.
Instead of awaiting it, could you assign it to a Task
? i.e. Task t = server.StartAsync()
?
The call to StartAsync is running the task, then returning that task back to you, so you don't need to await it.
_AcceptConnectionsTask = Task.Run(() => AcceptConnections(), _Token);
return _AcceptConnectionsTask;
FWIW, I just reproduced what you're seeing in Catalina, so it's not an OS issue.
I could yes, but then wouldn't I be back to square one as there wouldn't be a difference between server.Start()
& server.StartAsync()
? Is it not possible to fix the await issue?
You're right, this implementation still requires you to loop over IsListening
. Sorry about that, it didn't fix the root issue.
I'll move the call to start the listener into both Start
and StartAsync
. Give me a moment.
This one has the listener starting inside of Start
and StartAsync
. You should see IsListening == true
now after both. Please let me know if this works:
NuGet: https://www.nuget.org/packages/WatsonWebsocket/2.2.1.1
Commit: e502aa6
Updated and removed my while loop so thats great. Thanks for your work. However I still have a visual/semantics issue with not being able to await the async start;
Using the regular start within my (already async) function i get this back from Rider:
Which when I change to use the async overload
It pushes me to use the async tag (which when I add it, even in the new build) still fails to return and dead locks :(
Bah, you're right. I was thinking the main usage would be Task t = server.StartAsync()
, but a better approach is to await server.StartAsync()
with an optional CancellationToken
. One moment, updating.
This will (hopefully) do the trick:
NuGet: https://www.nuget.org/packages/WatsonWebsocket/2.2.1.2
Commit: 33a333c
Please let me know!
Still hangs unfortunatly :(
using System;
using System.Threading.Tasks;
using NUnit.Framework;
using WatsonWebsocket;
namespace WatsonServerHoldDemo
{
public class Tests
{
[Test]
[Timeout(5000)]
public async Task Test1()
{
var server = new WatsonWsServer
{
Logger = Console.WriteLine
};
await server.StartAsync();
var client = new WatsonWsClient("localhost", 9000, false)
{
Logger = Console.WriteLine
};
await client.StartAsync();
Console.WriteLine(client.Connected);
}
}
}
Looking into it now
I have recreated the issue in your test server project:
Line #147
static async void StartServer()
{
// _Server.Start();
Console.WriteLine("Starting...");
await _Server.StartAsync();
Console.WriteLine("Finished!");
}
Log
Server IP: [localhost]
Server port: [9000]
Use SSL: [y/N]? n
Command [? for help]: start
Starting...
[WatsonWsServer] starting http://localhost:9000/
Command [? for help]:
Starting is called and the server does indeed start up, but notice Finished!
is never called in the log.
Hi @Alig96 here's the challenge I'm running into while trying to devise an outcome that will allow you to await server.StartAsync()
.
StartAsync
calls AcceptConnections
which is a while loop. I wouldn't normally use a while loop in this case, but the call to listener.GetContextAsync()
is blocking, so I don't worry about CPU usage with it.
If I just return the Task
for AcceptConnections
, and it is awaited, it will block because the while loop is still running.
If I have AcceptConnections
kick off an unawaited Task
containing the while loop, it won't block, but then you won't have the actual Task
.
Would love your input on how you'd like me to proceed on this one. I'm of the opinion that having the Task
itself isn't super valuable, since you can pass in your own CancellationToken
on StartAsync
which will give you control over canceling it.
Thoughts?
The reason I'm wanting to be able to await the server starting is so that I can guarantee that everything after that line the server is online and ready to start accepting connections and sending data. The issue I had originally was that in the example in this since its a unit test and runs fast, after the calling server.Start()
the very next line would be client.Connect()
which would cause the client to fail connecting because the server is not ready yet because it takes a second or two to be ready. In a real life scenario, a client would probably never connect that fast but from a testing stance this is an issue.
You’re right that I’m not actually needing the actual task of AcceptConnections
back from the Start command. The internal task to me is an implementation detail and as a user of the library, I doubt I will ever require access to the actual AcceptConncections
task. I also believe that I (as a user) should not have control over the CancellationToken
. Seems like its leaking too much control like that, since there’s a Stop()
function, shouldn’t that be enough? Since the appeal of the library is be easy to get kickstarted. The ideal API in my eyes would be:
await server.Start()
// guaranteed server is ready
await server.Stop()
// guaranteed server is completely stopped
await client.Start()
// guaranteed client is connected/ready (if not perhaps an exception if it can’t start/connect)
await client.Stop()
// guaranteed client is completely stopped
Since those will all be asynchronous by default, the user can choose if they want it to run without blocking (by not adding the await tag) or blocking by adding the await tag which covers both the scenarios.
As a quick fix for me that I would be happy with, is just return a task that is ~1 second delay. However, long term for this project though, this would be a design flaw since the server could be ready way before that (which it usually is since it is a fast library, have not actually profiled it but can tell) and I would consider seeing if there’s a way to make this better.
This is excellent information. I agree that the Task
itself isn't really useful. The primary motivation for allowing the user to influence the CancellationToken
(it's an optional param) is in case they want to synchronize cancellation with other aspects of their application.
I'm going to test out a change wherein you can await StartAsync
and the returned Task
isn't tied to the AcceptConnections
loop. Reporting back soon.
This should do it. I'm going to do some more research into returning the actual Task without blocking should it be awaited.
NuGet: https://www.nuget.org/packages/WatsonWebsocket/2.2.1.3
Commit: f5756d8
Please close if this works for you, or let me know if any issues remain!
Ex:
await _Server.StartAsync();
Console.WriteLine("Server is listening: " + _Server.IsListening);
Fantastic, that's done the trick. Thanks alot for your help & quick responses and deploys.
I notice that you did end up going the delay
route. I hope you can refactor to return the actual task without blocking since that will make the application better in the long run.