doghappy / socket.io-client-csharp

socket.io-client implemention for .NET
MIT License
729 stars 125 forks source link

Unhandled exception in opened handler (SocketIO v2.3.1) #239

Open 23W opened 2 years ago

23W commented 2 years ago

Hi @doghappy Customer sent screenshot with unhandled exception. image and brief exception text part:

** Exception Text ** SocketIOClient.Exceptions.InvalidSocketStateException: Connection is not open. at SocketIOClient.WebSocketClient.ClientWebSocket.\<SendMessageAsync>d18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at SocketIOClient.WebSocketClient.ClientWebSocket.\<SendMessageAsync>d17.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at SocketIOClient.SocketIO.\<OpenedHandler>d__105.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

Seems exception occurs in OpenedHandler method at attempt to execute await Socket.SendMessageAsync(msg);

        private async void OpenedHandler(string sid, int pingInterval, int pingTimeout)
        {
            Id = sid;
            _pingInterval = pingInterval;
            string msg = Options.EioHandler.CreateConnectionMessage(Namespace, Options.Query);
            await Socket.SendMessageAsync(msg);
        }

Exception was not caught in OpenedHandler() or in OpenedProcessor.Process, which called this method as delegate. Can you please give recommendation how this situation is possible and how to fix code in 2.3.1 (this lib version is used in production and should be fixed).

doghappy commented 2 years ago

Seems exception occurs in OpenedHandler method at attempt to execute await Socket.SendMessageAsync(msg);

If the exception is indeed thrown here, it means that the WebSocket was disconnected shortly after the connection was established, causing some information of socket.io to not be sent.

If this problem occurs by accident, can it be fixed like this?

        private async void OpenedHandler(string sid, int pingInterval, int pingTimeout)
        {
            Id = sid;
            _pingInterval = pingInterval;
            string msg = Options.EioHandler.CreateConnectionMessage(Namespace, Options.Query);
            try
            {
                throw new Exception("TEST Exception");
                // await Socket.SendMessageAsync(msg).ConfigureAwait(false);
            }
            catch (Exception e)
            {
                ErrorHandler(e.Message);
                await ConnectAsync().ConfigureAwait(false);
            }
        }

btw, I will consider adding a better solution to the latest version.

23W commented 2 years ago

@doghappy Thank you for the advise, your suggestion seems works well. We age going to update our local fork of your library to fix the issue.

doghappy commented 2 years ago

Thank you for the advise, your suggestion seems works well. We age going to update our local fork of your library to fix the issue.

I suggest you based on this branch https://github.com/doghappy/socket.io-client-csharp/tree/v2.x

23W commented 2 years ago

Thank you for the advise, your suggestion seems works well. We age going to update our local fork of your library to fix the issue.

I suggest you based on this branch https://github.com/doghappy/socket.io-client-csharp/tree/v2.x

Do you mean to make a PR in v2.x branch with this fix?

doghappy commented 2 years ago

no.

~I suggest you based on this branch https://github.com/doghappy/socket.io-client-csharp/tree/v2.x~

I suggest you base on this branch https://github.com/doghappy/socket.io-client-csharp/tree/v2.x instead of downloading from the release page.

It is better if you are willing to submit a PR :)


btw, I would rather find the real reason, maybe it is the network reason, or something else? Do you have any suggestions?

In v3.x, I added a retry mechanism, I don't know which way is better.

DmitryBk commented 2 years ago

Of course, it is a rare case but I would suggest starting standard reconnection flow for this case

private async void OpenedHandler(string sid, int pingInterval, int pingTimeout)
{
    try
    {
        Id = sid;
        _pingInterval = pingInterval;
        string msg = Options.EioHandler.CreateConnectionMessage(Namespace, Options.Query);
        await Socket.SendMessageAsync(msg).ConfigureAwait(false);
    }
    catch (Exception e)
    {
        ErrorHandler(e.Message);

        Attempts++;
        await InvokeDisconnectAsync(e.Message).ConfigureAwait(false);
    }
}
doghappy commented 2 years ago

There is an if condition in InvokeDisconnectAsync

https://github.com/doghappy/socket.io-client-csharp/blob/d523efefaaf5f812d560aea4318d533200c10a5e/src/SocketIOClient/SocketIO.cs#L635-L655

When an exception is thrown in OpenedHandler, the value of Connected is false

At this point, WS has been established, but socket.io has not been established yet. The data sent by OpenedHandler is to establish socket.io connection on the basis of WS.

DmitryBk commented 2 years ago

~~It works for me with throw new Exception("TEST Exception"); It seems the OpenedHandler is called after the connection is established and after ConnectedHandler which set Connected = true~~

Sorry I was wrong, Connected is really false in that moment. My new suggestion just increments Attempts that OnReconnected event will be faired.

catch (Exception e)
{
    ErrorHandler(e.Message);

    Attempts++;
    await ConnectAsync().ConfigureAwait(false);
}