Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocketTransportInitiator.ConnectAsync can result in UnobservedTaskException #260

Closed
RusuIonut21 opened this issue Jun 11, 2024 · 7 comments

Comments

@RusuIonut21
Copy link

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
Copy link

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
Copy link
Member

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
Copy link
Member

Released 2.6.8

@RusuIonut21
Copy link
Author

@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
Copy link
Member

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 xinchen10 reopened this Jun 13, 2024
@xinchen10
Copy link
Member

@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
Copy link
Author

@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.

xinchen10 added a commit that referenced this issue Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants