mikerochip/unity-websocket

System.Net.WebSockets server causes error when calling "CloseAsync" on MikeSchweitzer.WebSocket in Unity editor

Closed this issue · 19 comments

crankywilson commented

This is a pretty minor problem, but I've found that if my server (I'm using the dotnet 9.0 standard System.Net.WebSockets in conjuction with the standard HttpListener on linux) attempts to close the connection to the Unity client websocket shortly after connecting, the call to CloseAsync() (on the server websocket) results in an exception:

Exception thrown: 'System.Net.WebSockets.WebSocketException' in System.Private.CoreLib.dll: 'The remote party closed the WebSocket connection without completing the close handshake.'
at System.Net.WebSockets.ManagedWebSocket.ThrowEOFUnexpected()
at System.Net.WebSockets.ManagedWebSocket.d__76.MoveNext()
at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder1.StateMachineBox1.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
at System.Net.WebSockets.ManagedWebSocket.d__65`1.MoveNext()

This only happens when I run my program in the Unity Editor. If I run it in a web browser with the Unity WebGL mode, everything is fine.

Most clients probably won't have a need for the server to close to the connection, (in fact I don't myself) so the issue is probably moot, and it seems to just be an editor mode issue, but still, it would be nice to fix this. ( I suspect the client websocket is missing some details on whatever the protocol specifies for handshakes when closing normally )

mikerochip commented

Thanks for the report. Hm. Not sure what you mean about "missing some details." This package is just using the websocket client built into .net for the editor path. Editor path is same as non-web builds which could explain why you're not seeing it on web.

Could you give me either a repro project or steps to set one up? Otherwise I can try to repro on my own server test project.

Are you basically closing the connection on the server side immediately after it opens?

mikerochip commented

OK I tested this out on my ASP test server. I suspect:

For the .Net path your server is getting a WebSocketException with error code 8 i.e. WebSocketError.ConnectionClosedPrematurely

For the web path, probably your client is generating error 1006, which is an abnormal closure (that's what I see in my chrome dev tools), and while your server is not getting an exception, it is probably getting a close status of 1001 i.e. WebSocketCloseStatus.EndpointUnavailable

I think the reason for these differences comes down to the quirks of differing implementations of web socket clients between .NET and Web browsers. These differences sadly exist in several areas (e.g. no header support in web, no built-in ping-pong support in web) and I think your server is just going to need to gracefully handle both cases.

This StackOverflow answer has a really good write up of why you get a 1006 on your web client.

However, Chrome will rarely report any close code 1006 reasons to the Javascript side. This is likely due to client security rules in the WebSocket spec to prevent abusing WebSocket. (such as using it to scan for open ports on a destination server, or for generating lots of connections for a denial-of-service attack).

Note that Chrome will often report a close code 1006 if there is an error during the HTTP Upgrade to Websocket (this is the step before a WebSocket is technically "connected"). For reasons such as bad authentication or authorization, or bad protocol use (such as requesting a subprotocol, but the server itself doesn't support that same subprotocol), or even an attempt at talking to a server location that isn't a WebSocket (such as attempting to connect to ws://images.google.com/)

I don't know what your server code looks like, but in my case, I repro'd this by closing the websocket immediately after it was accepted, which tracks with the SO answer above (the part "if there is an error during the HTTP Upgrade to Websocket").

So, I'm going to close this because I'm not sure that there is an actual issue in this repo, and if there is, I'm not sure what code would need to change in this repo. I'd be happy to see a PR if I'm missing something.

mikerochip commented

Not sure if this helps, but again, I suspect your server code is just going to have to change to handle both types of client corner cases gracefully, and similarly.

In my test server, I have some code to basically not report any errors or re-throw the exception if I get a WebSocketError.ConnectionClosedPrematurely e.g. like this:

using var webSocket = await context.WebSockets.AcceptWebSocketAsync();
try
{
    await DoStuffWithWebSocketAsync(webSocket);
}
catch (WebSocketException e)
{
    if (e.WebSocketErrorCode != WebSocketError.ConnectionClosedPrematurely)
        throw;
}

logger.LogInformation("Disconnect: {ConnectionId}", context.Connection.Id);
crankywilson commented

I didn't see any of these comments until just now... I don't know how to use github. I also do not use ASP - I find it way overcomplicated... I was just using the standard HttpListener which has methods for upgrading to websocket. I happen to be running on linux - if I put a tar.gz of my csproj and sources, could you use that?

I'm not in a position to look at anything at the moment. Perhaps I naively thought that whatever Chrome does -- that's the 'right' thing... I can tell you that I believe I didn't try to do the close on the server until the server thought the websocket connection was established (ie, after the upgrade)

I can try to see if the behavior is any different after a few messages have been exchanged. It seemed in my case, the server was expecting some kind of graceful response from the client after being asked to shutdown, but instead the connection was just abruptly terminating. Which would indeed be odd if is just using the same System.Net.WebSockets library I am on the server side. Both CloseOutputAsync and CloseAsync are being called before disposing?

mikerochip commented

I didn't see any of these comments until just now... I don't know how to use github.

You're doing fine as far as I can tell

I also do not use ASP - I find it way overcomplicated... I was just using the standard HttpListener which has methods for upgrading to websocket. I happen to be running on linux - if I put a tar.gz of my csproj and sources, could you use that?

Well, I'm not super interested in debating web server framework preferences. Your server's going to need to handle a variety of edge cases gracefully regardless of programming language or framework and from the sound of it, it's maybe not handling some cases gracefully at the moment. FWIW I'm running on Mac, and I was able to use my setup to reproduce the results as you described them in your original report. I'll speak on the sample code thing below.

I'm not in a position to look at anything at the moment. Perhaps I naively thought that whatever Chrome does -- that's the 'right' thing...

This is kind of a big topic. As far as whether Chrome or what any other browser does, is the 'right' thing - I disagree, it's actually the opposite in my mind. So the DotNet path throwing an exception with the ConnectionClosedPrematurely error is actually better than what the web path does (reporting an EndpointUnavailable close code with no server exception) because it is more specific and tells you exactly what's going on.

To be fair, I am relying on the stock websocket implementation from the browser's javascript engine (whichever browser your web build happens to be running in). It turns out the stock implementation is lacking in features compared to .NET Standard's System.Net.WebSockets.ClientWebSocket, which is what the DotNet path in this package uses (which is what is used in editor and in non-web builds). Sadly, the stock browser implementation is the easiest to use from Unity's javascript integration support (i.e. jslib files) because Unity's javascript integration doesn't support installing npm packages. Most websocket-heavy javascript web apps use ws, an npm package instead. I don't have personal experience with it, but I'd reach for that first if Unity had npm support since I know it can do things like ignore SSL.

However, the ideal thing that I'd do if I had a lot more time on my hands would be to roll my own javascript-based websocket implementation by hand that matched the behavior of the DotNet ClientWebSocket, NOT the other way around! That would be my ultimate preference and would eliminate differences in observed server responses. It's just an enormous amount of upfront and maintenance work that I can't take on at the moment, and would frankly not be much ROI.

The main reason why this package was written was because I didn't like the fact that the most popular open source websocket client package for Unity (NativeWebSocket) forced you to use async and conditionally-compiled-branch code for web and non-web. So that was my primary goal and was what I wanted to focus my time on solving. I have started to go down the road of shoring up missing features in web with the custom ping-pong support, but that wasn't too bad to build vs rewriting lower level network code in javascript.

I can tell you that I believe I didn't try to do the close on the server until the server thought the websocket connection was established (ie, after the upgrade)

I can try to see if the behavior is any different after a few messages have been exchanged. It seemed in my case, the server was expecting some kind of graceful response from the client after being asked to shutdown, but instead the connection was just abruptly terminating.

I'm not sure what to think... this statement sounds contradictory from the original report i.e. the original report stated

if my server ... attempts to close the connection to the Unity client websocket shortly after connecting

I guess the word "shortly" here is not clear enough, because in my interpretation, what that meant was that you were closing the connection in the next line after upgrading it. When I altered my server code to do that too, the result you reported is exactly what I saw on my machine. In my server test code, the next thing it does after upgrading to websocket is to call ReceiveAsync on it, so I have a suspicion your server code needs to be changed so that the websocket request handler does that too before an attempt to close it. I believe that's how handling of websockets is supposed to work, regardless of framework or programming language, but I'm not an expert at behavior intricacies of different server frameworks.

Just to set expectations, this feels pretty firmly in the territory of server code not handling all cases gracefully, which is out of scope of this repo since it does not include a server. I can consider open sourcing my test server at some point, but it's not super different from something like this:

Original:

using var webSocket = await context.WebSockets.AcceptWebSocketAsync();
try
{
    await DoStuffWithWebSocketAsync(webSocket);
}
catch (WebSocketException e)
{
    if (e.WebSocketErrorCode != WebSocketError.ConnectionClosedPrematurely)
        throw;
}

logger.LogInformation("Disconnect: {ConnectionId}", context.Connection.Id);

And this is more or less how I tested:

using var webSocket = await context.WebSockets.AcceptWebSocketAsync();
try
{
    await webSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, string.Empty, CancellationToken.None);
    //await DoStuffWithWebSocketAsync(webSocket);
}
catch (WebSocketException e)
{
    if (e.WebSocketErrorCode != WebSocketError.ConnectionClosedPrematurely)
        throw;
}

logger.LogInformation("Disconnect: {ConnectionId}", context.Connection.Id);

So yeah I think the best next step is to see some sample code snippet of what your server is doing. My preference is if you paste it in a reply to this thread as a code block. Would help others that way. If it's sensitive code and you don't want it to be in the open, you can email me.

crankywilson commented

Ok, here is a sample echo server that demonstrates the issue:

using System.Net;
using System.Net.WebSockets;

class Program()
{
  static void Main()
  {
    listener.Prefixes.Add("http://*:8001/");
    listener.Start();
    _ = Task.Run(AcceptConnections);

    Console.WriteLine("listening on " + listener.Prefixes.ElementAt(0));
    while (stayAlive) { Thread.Sleep(1000); }
    listener.Close();
  }

  static async Task AcceptConnections()
  {
    while (listener.IsListening)
    {
      var hc = await listener.GetContextAsync();
      if (!hc.Request.IsWebSocketRequest)
      {
        hc.Response.StatusCode = 400;
        hc.Response.Close();
        return;
      }

      var wsc = await hc.AcceptWebSocketAsync(null).ConfigureAwait(false);
      if (wsc != null)
      {
        Console.WriteLine($"New connection");
        WebSocket ws = wsc.WebSocket;

        Console.WriteLine("Waiting for msg");
        var read = await ws.ReceiveAsync(buf, CancellationToken.None);
        Console.WriteLine($"Got msg {read.Count}");
        await ws.SendAsync(buf, WebSocketMessageType.Text, true, CancellationToken.None);
        await ws.CloseAsync(WebSocketCloseStatus.NormalClosure, "Done", CancellationToken.None);
      
      }
      stayAlive = false;
    }
  }

  static byte[] buf = new byte[1000];
  static bool stayAlive = true;
  static HttpListener listener = new();
}

I set up my client in Unity to not send any messages until I press a key after I've been notified that the state is WebSocketState.Connected. So I know the websocket is clearly established (nothing 'premature')
But as soon as the server processes and echoes the message, it closes the websocket, which results in an exception.
It is certainly not difficult to catch and ignore, it just seems like closing shouldn't cause an exception.

crankywilson commented

BTW - I used the NativeWebSocket in my previous version of this program which I'm trying to upgrade. When I found this one, I was pretty happy that this exists now... So thank you for making this. I mean NativeWebSocket worked fine for me, but it felt pretty hacky setting things up. [In fact that was a remake of an all-javascript(/html) client (no Unity) which had its own issues. ]

(Also, yes, the WebSocketError code is indeed ConnectionClosedPrematurely.)

mikerochip commented

Cool thank you for the sample code, that's definitely different ordering from what I did. My test was calling CloseAsync basically right after the AcceptWebSocketAsync. I'll try out the equivalent of this in my ASP app first then if I don't have luck I'll use your exact code.

mikerochip commented

OK I'm pretty sure the problem is a bug in your server code due to confusing API naming and unclear docs for two different APIs that start with Close

So for your use case, you actually want to call:

await ws.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, "Done", CancellationToken.None);

not

await ws.CloseAsync(WebSocketCloseStatus.NormalClosure, "Done", CancellationToken.None);

The official documentation for CloseOutputAsync states that this is what is used to initiate a closure:

Initiates or completes the close handshake defined in the WebSocket protocol specification section 7.

The official documentation for CloseAsync is somewhat misleading, or maybe inadequate is the better term?

Closes the WebSocket connection as an asynchronous operation using the close handshake defined in the WebSocket protocol specification section 7

The intended usage is for completing a close handshake sent by the client. Something like this:

    WebSocketReceiveResult receiveResult;
    do
    {
        receiveResult = await webSocket.ReceiveAsync(new ArraySegment<byte>(receiveBuffer), CancellationToken.None);
        // send stuff
    } while (!receiveResult.CloseStatus.HasValue);

    // we got a close status from the client, so echo the result on the server
    await webSocket.CloseAsync(
        receiveResult.CloseStatus.Value,
        receiveResult.CloseStatusDescription,
        CancellationToken.None);
crankywilson commented

I did some Google-ing, and this is what I've concluded:

The "proper" way for either side (server or client) to initiate closing the websocket is to call CloseAsync()
The "proper" way to respond to the remote side starting a close is to call CloseOutputAsync().

The code examples I see that handle this correctly do this

 var result = await wsClient.ReceiveAsync(buf, CancellationToken.None);
 if (result.MessageType == WebSocketMessageType.Close)
        await wsClient.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, "", CancellationToken.None);

I think you'll find if you add this check right after your ReceiveAsync(), my server is happy without exception.

I believe this is what causes the "completing the close handshake" that the Exception was complaining about.

Agreed, the MS docs leave quite a bit to be desired.

mikerochip commented

Hm. Citations would help? I just asked my friendly neighborhood LLM and the response was basically...

CloseAsync() is used to initiate a close, but waits for the other side to respond with a CloseAsync()

CloseOutputAsync() is used to initiate a close, and in a fire and forget manner.

So yeah, makes sense why CloseOutputAsync() would work on your server code. My mistake! I'll reopen this and try adding the CloseAsync() to the ReceiveAsync() in the DotNet path.

Appreciate your patience with this, by the way! Thank you 🙏

crankywilson commented
mikerochip commented

You said "adding the CloseAsync() to the ReceiveAsync() in the DotNet
path." -- to be clear, I was thinking you should add "CloseOutputAsync"
when MessageType == WebSocketMessageType.Close "to the ReceiveAsync() in
the DotNet path." (on the Unity websocket client)

I don't think that signals the correct intention. As far as I can tell, CloseAsync() has to do with handshakes, and CloseOutputAsync() is for initiating a fire and forget close. Maybe it's functionally equivalent when a close is received, but it doesn't express the correct intention.

Actually my existing single CloseAsync() call should probably be changed to CloseOutputAsync() (this one here) since the intention of that call is not to wait for the server to respond.

Either way, I'm working on it. I'm taking the opportunity to refactor my receive loop in the DotNet path since it's kind of a mess and hard to read ATM so it's taking a smidge longer than expected.

mikerochip commented

Still need to test but if you want to double check my WIP main...fix-dotnet-close

crankywilson commented
crankywilson commented
mikerochip commented

Hi Mike,

I have coded into my app the auto-closing of the (client) websocket in
the Unity editor when you press the stop button. It would be nice if
the library did this for me... Especially because it seems I need to
wait at least one update frame before the call to Disconnect() seems
to do anything. I suspect if the library handled this for me, it
could directly access the Close call immediately without having to
wait frame(s), which would keep the stop button more responsive...

So, feature request?

(If I don't call Disconnect before stopping, the server thinks the
connection is still open and then eventually throws an exception later
on. And as you know, I don't like seeing exceptions thrown in normal
use cases. And it seems to be another difference between javascript
mode where closing the page does the "right thing" IMO)

Hi. This is a new issue and unrelated to the current one; please file a new issue for new issues.

mikerochip commented

also, as far as I can tell, the latest version does not have a way to
close the websocket in Windowed app without causing an exception on
the server.
…

Windowed app? Do you mean when you make a win/mac/linux desktop build?

Also, using my test server and my test client, calling Disconnect() from my test client does not yield a server exception, so I need more info.

mikerochip commented

Locking this since this issue has been fixed