dotnet / WatsonTcp

WatsonTcp is the easiest way to build TCP-based clients and servers in C#.
MIT License
599 stars 117 forks source link

WatsonTcpServer.ListClients() is empty if called "too early" #223

Open Laiteux opened 2 years ago

Laiteux commented 2 years ago

Hey!

server.Start();
client.Connect();

Console.WriteLine(server.ListClients().Count());
await Task.Delay(1); // Thread.Sleep(1) also works
Console.WriteLine(server.ListClients().Count());

This will output:

0
1

This is very problematic as one might not expect server.ListClients() to return an empty enumerable, especially if they're not aware of this issue.

I'm not sure why a 1ms delay seems to fix it. Are some async events (registering the client on server side) not awaited/fired on time or something?

Why does this happen and could it be fixed?

Laiteux commented 2 years ago

Update: Does not always seem to work with a 1ms delay. Had to increase it a little to make sure it would work everytime.

jchristn commented 2 years ago

Hi Matt, the reason is that the server has a background loop in AcceptConnections to, well... accept connections :)

When a connection is accepted, there are certain things that are done synchronously (prior to the loop starting so it can accept another connection) vs in a background task (thereby allowing the server to accept another connection).

You'll notice there is an unawaited task in AcceptConnections to StartTls (if using SSL/TLS) and then to FinalizeConnection (in both SSL/TLS and non-SSL/TLS cases).

In FinalizeConnection the client is finally added to the client list on the server. This can be moved up to the main AcceptConnections loop, but will need to (carefully) amend failure cases from there to reliably remove the client from the list if there are any issues. I'll see if I can put something together on this.

Laiteux commented 2 years ago

Sounds good! Thanks for the explanation. No rush on this, it's something I randomly noticed but I doubt many people will encounter this issue anyway.

jchristn commented 2 years ago

Could you give v5.0.6 a try?

NuGet: https://www.nuget.org/packages/WatsonTcp/5.0.6 Commit: https://github.com/jchristn/WatsonTcp/commit/d5becb56cc16e907fc906542e0e7dd8780b5fb84

Laiteux commented 2 years ago

Sadly, the same issue remains.

Fiddle: https://dotnetfiddle.net/fK8snH

jchristn commented 2 years ago

Yeah, not sure how I'm going to be able to fix that. There's very little code between the server accepting the connection and the client metadata being added to the client list. This is a biproduct of the fact that the client is returning faster from BeginConnect() (and everything else it does within Connect() than the server does from the first few steps of AcceptConnections().

Laiteux commented 2 years ago

Yeah, very understandable.

You can label this issue if you wish and keep it for later or whatever, I guess it'll be fine for now. Especially if I'm the first one reporting about it.

In a real server<->client scenario, this probably won't be important anyway because of the fact there is already ping between everything.

This was found because I ran some tests locally, but I can hardly think of a situation where you'd need to call ListClients() less than a few milliseconds after a client connects.