Skip to content

Avoid unobserved tasks in WebSocketsTransport #12315

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.

Sign up for GitHub

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

Merged
BrennanConroy merged 3 commits into from
Jul 25, 2019
Merged

Avoid unobserved tasks in WebSocketsTransport #12315

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e54457
Avoid unobserved tasks in WebSocketsTransport
BrennanConroy Jul 18, 2019
31ed1bc
log
BrennanConroy Jul 24, 2019
bbd47de
fb
BrennanConroy Jul 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ private async Task ProcessSocketAsync(WebSocket socket)

if (trigger == receiving)
{
// Observe exception if there is one to avoid unobserved tasks
_ = receiving.Exception;

@halter73 halter73 Jul 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't these exceptions matter? Should we at least be logging them?

@analogrelay analogrelay Jul 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should, but to no higher than Debug since they will include client disconnects.

@davidfowl davidfowl Jul 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug ftw

@BrennanConroy BrennanConroy Jul 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should never actually have exceptions in them, I was just being paranoid. We have a try catch around both sending and receiving so the only exceptions that could escape would be Pipe.Complete throwing, logger throwing, or socket.CloseOutputAsync (this would be the one most likely to throw).

I'll just wrap socket.CloseOutputAsync and log it if it throws.

@halter73 halter73 Jul 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of all the _ = blah.Exception lines then?

The mere existence of these lines communicates to other developers that these tasks could have completed with an exception. I don't want to risk seeing this pattern copied more widely. The more places this pattern is used, the more likely it will be copied somewhere else.

@BrennanConroy BrennanConroy Jul 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm good with removing them

@halter73 halter73 Jul 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. If for whatever reason we are still concerned about something unexpected throwing, we could also replace the _ = blah.Exception with await trigger. That way if there was an unexpected error, WebSocketTransport.StopAsync() would rethrow it.


// We're waiting for the application to finish and there are 2 things it could be doing
// 1. Waiting for application data
// 2. Waiting for a websocket send to complete
Expand All @@ -176,13 +179,19 @@ private async Task ProcessSocketAsync(WebSocket socket)
}
else
{
// Observe exception if there is one to avoid unobserved tasks
_ = sending.Exception;

// Cancel the timeout
delayCts.Cancel();
}
}
}
else
{
// Observe exception if there is one to avoid unobserved tasks
_ = sending.Exception;

// We're waiting on the websocket to close and there are 2 things it could be doing
// 1. Waiting for websocket data
// 2. Waiting on a flush to complete (backpressure being applied)
Expand Down Expand Up @@ -269,10 +278,6 @@ private async Task StartReceiving(WebSocket socket)
if (!_aborted)
{
_application.Output.Complete(ex);

// We re-throw here so we can communicate that there was an error when sending
// the close frame
throw;

@BrennanConroy BrennanConroy Jul 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main culprit for unobserved tasks.

An alternative solution would be to keep this and actually observe the exception and set a private field to let the send loop know to send an internal server error instead of possibly a normal exit code.
https://github.com/aspnet/AspNetCore/blob/010ffe612150d9a3f5b78f2ad3ffcee3aedef369/src/SignalR/common/Http.Connections/src/Internal/Transports/WebSocketsTransport.cs#L272

}
}
finally
Expand Down
16 changes: 12 additions & 4 deletions src/SignalR/common/Http.Connections/src/Internal/Transports/WebSocketsServerTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ public async Task ProcessSocketAsync(WebSocket socket)

if (trigger == receiving)
{
// Observe exception if there is one to avoid unobserved tasks
_ = receiving.Exception;

Log.WaitingForSend(_logger);

// We're waiting for the application to finish and there are 2 things it could be doing
Expand All @@ -102,12 +105,18 @@ public async Task ProcessSocketAsync(WebSocket socket)
}
else
{
// Observe exception if there is one to avoid unobserved tasks
_ = sending.Exception;

delayCts.Cancel();
}
}
}
else
{
// Observe exception if there is one to avoid unobserved tasks
_ = sending.Exception;

Log.WaitingForClose(_logger);

// We're waiting on the websocket to close and there are 2 things it could be doing
Expand All @@ -130,6 +139,9 @@ public async Task ProcessSocketAsync(WebSocket socket)
}
else
{
// Observe exception if there is one to avoid unobserved tasks
_ = receiving.Exception;

delayCts.Cancel();
}
}
Expand Down Expand Up @@ -189,10 +201,6 @@ private async Task StartReceiving(WebSocket socket)
if (!_aborted && !token.IsCancellationRequested)
{
_application.Output.Complete(ex);

// We re-throw here so we can communicate that there was an error when sending
// the close frame
throw;
}
}
finally
Expand Down