jchristn / WatsonWebsocket

A simple C# async websocket server and client for reliable transmission and receipt of data
MIT License
277 stars 53 forks source link

Connection Status while connecting #129

Closed codengine closed 1 year ago

codengine commented 1 year ago

Hi, so I'm using the latest version of your library and noticed that the event handling of connect/disconnect is not working properly.

If I connect using "Client.StartWithTimeoutAsync()" and have setup event handlers like

        client.ServerConnected += OnConnected;
        client.ServerDisconnected += OnDisconnected;

I will get multiple events, like hundreds, like this

Connected
Disconnected
Connected
Disconnected
Connected
Disconnected
...

Which is undesirable, especially if those event handlers are used to trigger certain things or as a source of binding it to a button / status text.

In my humble opinion the events should only be invoked if the connection is truly "connected" or "disconnected".

jchristn commented 1 year ago

Hi, I just tested this using the Test.Client and Test.Server projects (I modified the client to use StartWithTimeoutAsync) and am unable to reproduce this. Can you reproduce this behavior with these projects, or, could you supply a project that shows this issue?

codengine commented 1 year ago

I forgot to mention: The server is not started, as I just wanted to test the behavior of the client if it is not up.

jchristn commented 1 year ago

Ok, that's good to know. Thanks. I couldn't reproduce this in the Test.Client and Test.Server projects. I modified the client to use _Client.StartWithTimeoutAsync(30).Wait(); and both ServerConnected and ServerDisconnected are set.

Can you reproduce this using these two projects? Or can you share code?

Also what operating system are you running on, and, what runtime/framework (including version)?

codengine commented 1 year ago

Ok, that's good to know. Thanks. I couldn't reproduce this in the Test.Client and Test.Server projects. I modified the client to use _Client.StartWithTimeoutAsync(30).Wait(); and both ServerConnected and ServerDisconnected are set.

Can you reproduce this using these two projects? Or can you share code?

Also what operating system are you running on, and, what runtime/framework (including version)?

Sure, I will share the details later today.

codengine commented 1 year ago

I've created a test project here: https://github.com/codengine/WatsonTest

The thing is: It does not always happen. When I started my computer this morning and opened the same project as yesterday, there was no issue. Then after some runs it happened again.

Same with the example project. I created it, ran it a couple of times - Issue was there. I started OBS to record the behavior - no issue anymore.

So what happens is that this block:

while (sw.Elapsed < timeOut)

Is called multiple times until the timeout is reached. During the attempts where the issue does not happen, I can see that the Task status is "Faulted" grafik

But I remember that in those cases where I had the Connect/Disconnect spam the Task status was "Reached Completion". What I also noticed is that when the issue occurs, the call to

watsonWsClient._ClientWs.ConnectAsync

completes almost immediately, whereas if it doesnt happen, there is like a 2-3 second delay between connection attempts.

codengine commented 1 year ago

I'm running Windows 10 22H2, "dotnet --version" shows 7.0.302. The example project was created for C# 6.0.

jchristn commented 1 year ago

Just curious - the code you're showing above doesn't match that of the latest version. The AfterConnect block in the latest is:

private void AfterConnect(Task task)
{
    if (task.IsCompleted)
    {
        if (_ClientWs.State == WebSocketState.Open)
        {
            Task.Run(() =>
            {
                Task.Run(() => DataReceiver(), _Token);
                ServerConnected?.Invoke(this, EventArgs.Empty);
            }, _Token);
        }
    }
}

Which is logically different than yours. Or, it could be decompiling the library to what the library code compiled to?

I'm not understanding how the call to fire ServerConnected could even happen. It only should happen 1) when the task is completed and 2) if the websocket state is open.

I'll put together a patch that amends this to also ensure task.IsFaulted == false, one moment.

jchristn commented 1 year ago

Ok, changed AfterConnect to:

private void AfterConnect(Task task)
{
    if (!task.IsFaulted && task.IsCompleted)
    {
        if (_ClientWs.State == WebSocketState.Open)
        {
            Task.Run(() =>
            {
                Task.Run(() => DataReceiver(), _Token);
                ServerConnected?.Invoke(this, EventArgs.Empty);
            }, _Token);
        }
    }
}

Could you try v4.0.10?

NuGet: https://www.nuget.org/packages/WatsonWebsocket/4.0.10 Commit: https://github.com/jchristn/WatsonWebsocket/commit/7181bf109358ef99e63dce3e064d1e4db86b57ad

codengine commented 1 year ago

I will try it. Hopefully I can reproduce the issue. EDIT: It doesn't happen with 4.0.9 right now, I'll call back when I have no information.

codengine commented 1 year ago

Suddenly it happened again, even with 4.0.10. I was able it to capture it on a video: https://www.youtube.com/watch?v=gUm9aZOVa7I

Moreover, here is a dump of the task within the AfterComplete method:

task = {AsyncTaskMethodBuilder<VoidTaskResult>.AsyncStateMachineBox<ClientWebSocket.<ConnectAsyncCore>d__15>} Id = 6, Status = RanToCompletion, Method = {null}, Result = System.Threading.Tasks.VoidTaskResult
 AsyncState = {object} null
 CancellationToken = {CancellationToken} IsCancellationRequested = false
 CapturedContext = ExecutionContext
 CompletedEvent = {ManualResetEventSlim} Set = true
 Context = {ExecutionContext} null
 CreationOptions = {TaskCreationOptions} None
 DebuggerDisplayMethodDescription (Task) = {string} "{null}"
 DebuggerDisplayMethodDescription = {string} "{null}"
 DebuggerDisplayResultDescription = {string} "System.Threading.Tasks.VoidTaskResult"
 Exception = {AggregateException} null
 ExceptionRecorded = {bool} false
 ExecutingTaskScheduler = {TaskScheduler} null
 Id = {int} 6
 IsCanceled = {bool} false
 IsCancellationAcknowledged = {bool} false
 IsCancellationRequested = {bool} false
 IsCompleted = {bool} true
 IsCompletedSuccessfully = {bool} true
 IsDelegateInvoked = {bool} false
 IsExceptionObservedByParent = {bool} false
 IsFaulted = {bool} false
 IsWaitNotificationEnabled = {bool} false
 IsWaitNotificationEnabledOrNotRanToCompletion = {bool} false
 MoveNextAction = Action
 Options = {TaskCreationOptions} 1024
 ParentForDebugger = {Task} null
 ResultOnSuccess = VoidTaskResult
 ShouldNotifyDebuggerOfWaitCompletion = {bool} false
 StateFlags = {Task.TaskStateFlags} RanToCompletion | WaitingForActivation | CompletionReserved
 StateFlagsForDebugger = {int} 117441536
 StateMachine = ClientWebSocket.<ConnectAsyncCore>d__15
 Status = {TaskStatus} RanToCompletion
 System.IAsyncResult.AsyncWaitHandle = ManualResetEvent
 System.IAsyncResult.CompletedSynchronously = {bool} false
 _moveNextAction = Action
 m_action = {Delegate} null
 m_contingentProperties = Task.ContingentProperties
 m_continuationObject = object
 m_result = VoidTaskResult
 m_stateFlags = {int} 117441536
 m_stateObject = {object} null
 m_taskId = {int} 6
 m_taskScheduler = {TaskScheduler} null
codengine commented 1 year ago

And this is the state of the _ClientWs:

_ClientWs = ClientWebSocket
 CloseStatus = {WebSocketCloseStatus?} null
 CloseStatusDescription = {string} null
 ConnectedWebSocket = ManagedWebSocket
 Options = ClientWebSocketOptions
 State = {WebSocketState} Open
 SubProtocol = {string} null
 _innerWebSocket = WebSocketHandle
 _state = {int} 2
jchristn commented 1 year ago

Thanks for taking the time to do this. This is really strange. These lines:

 IsCompleted = {bool} true
 IsCompletedSuccessfully = {bool} true

Seem to indicate that it was able to connect successfully (is there another webserver running?). I don't know how, from the parameters you've shown, to distinguish that this was a failed connection attempt. It appears correct. Are you now able to reproduce it consistently or no?

codengine commented 1 year ago

I'm so sorry... I should have checked if something was listening on that port. And yes, there is an application listening on the port... This can be closed :D

But still, I think the check if the task is faulted can stay. Sorry to waste your time :)

jchristn commented 1 year ago

No waste of time at all! Please let me know if I can help in any way going forward. Cheers