BarRaider / obs-websocket-dotnet

C# .NET library to communicate with an obs-websocket server
MIT License
224 stars 104 forks source link

[BUG] `Disconnected` event gets incorrect values #103

Open DmitriySalnikov opened 2 years ago

DmitriySalnikov commented 2 years ago

Issue Type

Describe the bug The Disconnected event returns only values from WebSocketCloseStatus (1000-1011) and marks them as ObsCloseCodes. Whereas the OBS protocol returns the values 0, 4000, 4002, 4003-4012.

To Reproduce Steps to reproduce the behavior:

  1. Connect to OBS
  2. Place the breakpoint on OBSWebsocket.OnWebsocketDisconnect()
  3. Close OBS
  4. Check the variable d.CloseStatus

Expected behavior Probably I would like to get either a combined enum from the OBS protocol and the WebSocket enum, or get some kind of remap of WebSocket values to the values from the OBS protocol enum.

Screenshots image

image

Versions OBS Version: 28.0.1 OBS WebSocket Version: OBS WebSocket Dotnet (this library) Version: 257e7b48e1c365fcf2ea3de9f156ded2bf8f5247 OS: Windows 11

BarRaider commented 2 years ago

I'm not sure what exactly is the problem you're mentioning. It definitely does return codes 4000+ as we use it to determine Auth failure

Can you make a small sample code replicating the issue?

DmitriySalnikov commented 2 years ago

I got error 1001, as in the screenshot, using the original TestClient project. If the error was in the range of 4000-4012, then it would be displayed in text, not numbers..

image image

Steps to reproduce the behavior:

image

BarRaider commented 2 years ago

1000 is not a OBS Close Code so it won't return an enum value. You can see the valid codes here: https://github.com/obsproject/obs-websocket/blob/master/docs/generated/protocol.md#websocketopcode

The other codes are internal Websocket codes

DmitriySalnikov commented 2 years ago

The other codes are internal Websocket codes

Yes, I understood that. It's just not documented. It seemed to me that this could be done more explicitly.

At the moment I have to do something like this?

if ((int)e.ObsCloseCode < 4000)
{
    var ee = (System.Net.WebSockets.WebSocketCloseStatus)e.ObsCloseCode;
    switch (ee)
    {
        case System.Net.WebSockets.WebSocketCloseStatus.NormalClosure:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.EndpointUnavailable:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.ProtocolError:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.InvalidMessageType:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.Empty:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.InvalidPayloadData:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.PolicyViolation:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.MessageTooBig:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.MandatoryExtension:
            break;
        case System.Net.WebSockets.WebSocketCloseStatus.InternalServerError:
            break;
    }
}
else
{
    switch (e.ObsCloseCode)
    {
        case ObsCloseCodes.DontClose:
            break;
        case ObsCloseCodes.UnknownReason:
            break;
        case ObsCloseCodes.MessageDecodeError:
            break;
        case ObsCloseCodes.MissingDataField:
            break;
        case ObsCloseCodes.InvalidDataFieldType:
            break;
        case ObsCloseCodes.InvalidDataFieldValue:
            break;
        case ObsCloseCodes.UnknownOpCode:
            break;
        case ObsCloseCodes.NotIdentified:
            break;
        case ObsCloseCodes.AlreadyIdentified:
            break;
        case ObsCloseCodes.AuthenticationFailed:
            break;
        case ObsCloseCodes.UnsupportedRpcVersion:
            break;
        case ObsCloseCodes.SessionInvalidated:
            break;
        case ObsCloseCodes.UnsupportedFeature:
            break;
    }
}

If the answer is yes, then I think you can close this question.

ProbablePrime commented 2 years ago

I too am seeing 1000, 1001 etc coming in this handler from e.ObsCloseCode

In my case, connect to OBS, then force close OBS -> 1001.

You asked for a reproduction:

using OBSWebsocketDotNet;
using OBSWebsocketDotNet.Communication;

namespace OBSWebsocketTest
{
    public class Program
    {
        public static Task Main(string[] args) => new Program().MainAsync();
        public async Task MainAsync()
        {
            OBSWebsocket obs = new OBSWebsocket();

            obs.Connected += Obs_Connected;
            obs.Disconnected += Obs_Disconnected;
            obs.ConnectAsync("ws://127.0.0.1:4455", "");

            // Block this task until the program is closed.
            await Task.Delay(-1);
        }
        void Obs_Connected(object? sender, EventArgs e)
        {
            Console.WriteLine("Connected");
        }

        void Obs_Disconnected(object? sender, ObsDisconnectionInfo e)
        {
            Console.WriteLine(e.ObsCloseCode);
        }

Running this, with OBS open, waiting till you see connected and then closing OBS will result in a 1001 close code.

I think the original poster is saying that the additional codes(<4000) are not documented in the library and that the type(ObsCloseCodes) does not handle them. This leads to a confusing experience when consuming this client.

I guess that when I consume a client application, I expect an Enum value to not contain values that are not defined in the Enum.

If I were writing this library I would probably either:

  1. Extends ObsCloseCodes from another Enum type that does contain codes <4000
  2. Change the handling/invocation of the disconnected handler to not send <4000 codes via the ObsCloseCodes type.

In my case, I can handle this with some extra code similar to the original poster, but I do agree with them it is confusing.

BarRaider commented 2 years ago

I will address this moving forward