Azure / azure-amqp

AMQP C# library
Other
94 stars 70 forks source link

WebSocketTransportInitiator.ConnectAsync can result in UnobservedTaskException #260

Closed RusuIonut21 closed 3 months ago

RusuIonut21 commented 3 months ago

The problem seems to be fixed on main branch with this commit:

var task = new TimeoutTaskSource<ClientWebSocket>(
  cws,
  s => s.ConnectAsync(this.settings.Uri, CancellationToken.None),
  static s => s.Abort(),
  timeout).Task;

This commit was released in v2.4.9 and v3.0.0-preview, but the releases after v2.4.9 have the previous version of this:

Task task = cws.ConnectAsync(this.settings.Uri, CancellationToken.None).WithTimeout(timeout, () => "timeout");

@xinchen10 Was this revert desired or is a mistake? Can I get a release where cws.Abort() is called in case of timeout?

woppa684 commented 3 months ago

It just seems as if 2.4.10 (where the files moved) didn't take these changes along and instead used a version of this file from a year before (2017). Anyway, having UnobservedTaskExceptions is blocking our development indeed and it is very hard to work around without having to implement a lot ourselves.

I'm also curious to know what the plan is for v3 (where this change is also in apparently). Also, we use this dependency transitively from the Azure EventHubs library, where they would also need to update to the new version then I guess.

xinchen10 commented 3 months ago

I believe it was a mistake due to large amount of code movement. We are fixing it. Once a new package is released, you can either add an explicit reference to this package in your project to pick up the change, or wait for next EH SDK update.

xinchen10 commented 3 months ago

Released 2.6.8

RusuIonut21 commented 3 months ago

@xinchen10 The UnobservedTaskException still happens after the fix. The problem is the same, the task returned from cws.ConnectAsync(this.settings.Uri, CancellationToken.None) is not "observed". The cws.Abort() call cancels the opetation, an exception is thrown inside ConnectAsync method, so the received task is faulted.

Task task = cws.ConnectAsync(this.settings.Uri, CancellationToken.None).WithTimeout(timeout, () => "Client WebSocket connect timed out");

One solution would be something like this:

Task connectTask = cws.ConnectAsync(this.settings.Uri, CancellationToken.None);
connectTask.ContinueWith(t => 
{
    if (t.IsFaulted)
    {
        _ = t.Exception;
    }
});

Task task = connectTask.WithTimeout(timeout, () => "Client WebSocket connect timed out");

Another possible fix would be something like this:

CancellationTokenSource cts = new CancellationTokenSource(timeout);
cws.ConnectAsync(this.settings.Uri, cts.Token).ContinueWith(t =>
{
    if (t.IsFaulted)
    {
        callbackArgs.Exception = t.Exception.InnerException;
    }
    else if (t.IsCanceled)
    {
        callbackArgs.Exception = new OperationCanceledException(); // or new TimeoutException("Client WebSocket connect timed out");
    }
    else
    {
        callbackArgs.Transport = new WebSocketTransport(cws, this.settings.Uri);
    }

    callbackArgs.CompletedCallback(callbackArgs);
});
xinchen10 commented 3 months ago

I misunderstood the problem. The issue is in WithTimeout extension method, where the inner Faulted task is not observed. Instead of fixing the extension method, I will remove it completely.

xinchen10 commented 3 months ago

@RusuIonut21 I uploaded package 2.6.9 to nuget.org. It is currently not listed in search results but it can be referenced explicitly with version "2.6.9". I have done limited testing with the websocket transport but it was not easy for me to simulate the error conditions. It would be great if you could try it to see if it resolved the issues you observed. Thanks.

RusuIonut21 commented 3 months ago

@xinchen10 I tried the latest release (2.6.9) and the problem is solved, it works correctly now. Thank you for quickly fixing this issue.