OperationCanceledException being thrown on ISocket.ConnectAsync and ISocket.Dispose

[alexbfreeman]

Hello everyone,

I'm trying to close a socket connection by calling the CloseAsync method of Socket.cs. This exception is being thrown and I think I know the reason

System.OperationCanceledException: The operation was canceled.
   at System.Threading.CancellationToken.ThrowOperationCanceledException()
   at System.Threading.CancellationToken.ThrowIfCancellationRequested()
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.GetResult(Int16 token)
   at System.Net.Security.SslStream.ReadAsyncInternal[TIOAdapter](TIOAdapter adapter, Memory`1 buffer)
   at Nakama.Ninja.WebSockets.Internal.BinaryReaderWriter.ReadExactly(Int32 length, Stream stream, ArraySegment`1 buffer, CancellationToken cancellationToken) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/Ninja.WebSockets/Internal/BinaryReaderWriter.cs:line 49
   at Nakama.Ninja.WebSockets.Internal.WebSocketFrameReader.ReadAsync(Stream fromStream, ArraySegment`1 intoBuffer, CancellationToken cancellationToken) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/Ninja.WebSockets/Internal/WebSocketFrameReader.cs:line 50
   at Nakama.Ninja.WebSockets.Internal.WebSocketImplementation.ReceiveAsync(ArraySegment`1 buffer, CancellationToken cancellationToken) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/Ninja.WebSockets/Internal/WebSocketImplementation.cs:line 122
   at Nakama.Ninja.WebSockets.Internal.WebSocketImplementation.ReceiveAsync(ArraySegment`1 buffer, CancellationToken cancellationToken) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/Ninja.WebSockets/Internal/WebSocketImplementation.cs:line 142
   at Nakama.Ninja.WebSockets.Internal.WebSocketImplementation.ReceiveAsync(ArraySegment`1 buffer, CancellationToken cancellationToken) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/Ninja.WebSockets/Internal/WebSocketImplementation.cs:line 195
   at Nakama.WebSocketAdapter.ReceiveLoop(WebSocket webSocket, CancellationToken cancellationToken) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/WebSocketAdapter.cs:line 175
   at Nakama.WebSocketAdapter.Connect(Uri uri, Int32 timeout) in /Users/alfreeman/Github/nakama-dotnet/src/Nakama/WebSocketAdapter.cs:line 115

This is the Connect method from WebSocketAdapter.cs

public async void Connect(Uri uri, int timeout)
        {
            if (_webSocket != null)
            {
                ReceivedError?.Invoke(new SocketException((int) SocketError.IsConnected));
                return;
            }

            _cancellationSource = new CancellationTokenSource();
            _uri = uri;
            IsConnecting = true;

            var clientFactory = new WebSocketClientFactory();
            try
            {
                var cts = new CancellationTokenSource(TimeSpan.FromSeconds(timeout));
                var lcts = CancellationTokenSource.CreateLinkedTokenSource(_cancellationSource.Token, cts.Token);
                using (_webSocket = await clientFactory.ConnectAsync(_uri, _options, lcts.Token))
                {
                    IsConnected = true;
                    IsConnecting = false;
                    Connected?.Invoke();

                    await ReceiveLoop(_webSocket, _cancellationSource.Token);
                }
            }
            catch (TaskCanceledException)
            {
                // No error, the socket got closed via the cancellation signal.
            }
            catch (ObjectDisposedException)
            {
                // No error, the socket got closed.
            }            
            catch (Exception e)
            {
                ReceivedError?.Invoke(e);
            }
            finally
            {
                Close();
                Closed?.Invoke();
            }
        }

It is not catching the case where a OperationCanceledException is thrown because a cancellation request has been made. Is this intentional? Wouldn't it be more appropriate to check if a cancellation has been requested and not throw an error if so? For example

           catch (OperationCanceledException ex)
            {
                if(!ex.CancellationToken.IsCancellationRequested)
                {
                    ReceivedError?.Invoke(ex);
                }
            }

[lugehorsam]

Hey @alexbfreeman, I can understand why you may expect an error not to be thrown since you are the one initiating the cancellation. However, this is a bit of a double-edged sword because other users may actually expect it to throw. Also, there is not always a way to detect what the source of a cancellation is from the perspective of the Task.

In a gray area like this we defer to Microsoft which recommends that you throw a cancellation exception if a user cancels their own Task: https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/task-cancellation

Note that you can check the status on the cancelled Task upon catching the exception. It will be a Canceled status. So, you at least have that information.

[lugehorsam]

Actually @alexbfreeman I think there may be a few related issues with this block of code that could be causing unexpected behavior, since you mention an OperationCanceledException and not a TaskCanceledException. I'll circle back with the team.

[lugehorsam]

@alexbfreeman I wanted to confirm that you are closing the connection while the socket is connecting? If so, this is expected behavior. Otherwise it may not be.

[alexbfreeman]

Yes, it's an OperationCanceledException. I can't properly catch the exception on my code because it happens in the inner loop (ReceiveLoop), which is inside an async void method (see https://docs.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming on why we should avoid async void methods except if it's an event handler).

I could wait for a OnSocketError event and check if it's an OperationCanceledException, but there might be other reasons for the exception other than me closing the connection.

And no, I'm not closing the connection while the socket is connecting.

Furthermore, we would like to get some metrics on the connection time, but the connection is happening inside an async void, so we can't properly measure it on our code.

[alexbfreeman]

Could MemoryStream.ReadAsync be returning an OperationCanceledException instead of a canceled task?

[lugehorsam]

It's possible. I'll have a look later today or Monday to see if there's something I can simplify and make more predictable as to what exceptions are thrown when.

[lugehorsam]

@alexbfreeman Agreed on the async void point -- I'll make that change. The original author is on vacation for a week so I'll probably wait for his review in case he had a particular behind the async void.

What's odd to me though is I can't reproduce the throwing of an exception while closing a socket in my .NET test, regardless of whether or not the socket lifecycle methods return Task or not.

For example: https://github.com/heroiclabs/nakama-dotnet/blob/master/tests/Nakama.Tests/Socket/WebSocketTest.cs#L72 has always passed for me, again regardless of Task or void. If an exception were thrown, the test would not pass.

Perhaps this exception is thrown when using Unity's synchronization context but not the standard .NET one.

[lugehorsam]

I'm also having trouble triggering the exception to be thrown in Unity. The socket connects and closes without any errors or exceptions. If you have an example reproduction though, that would be great.

[Donnotron666]

Hi! Just weighing in! This exception has been plaguing our project for a bit, even though I've never been able to reproduce it in Unity- it's our number one exception that appears in our automated exception tracker.

[lugehorsam]

@Donnotron666 A fix for this issue is under team review and will be included in the next Nakama release.

[GaNacereddine]

@lugehorsam Can you share if this was ever fixed?

[lugehorsam]

@GaNacereddine yes it should be fixed. I'll close out this issue.