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 all commits
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
2 changes: 1 addition & 1 deletion src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.Log.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ private static class Log
LoggerMessage.Define(LogLevel.Information, new EventId(16, "ClosingWebSocket"), "Closing WebSocket.");

private static readonly Action<ILogger, Exception> _closingWebSocketFailed =
LoggerMessage.Define(LogLevel.Information, new EventId(17, "ClosingWebSocketFailed"), "Closing webSocket failed.");
LoggerMessage.Define(LogLevel.Debug, new EventId(17, "ClosingWebSocketFailed"), "Closing webSocket failed.");

private static readonly Action<ILogger, Exception> _cancelMessage =
LoggerMessage.Define(LogLevel.Debug, new EventId(18, "CancelMessage"), "Canceled passing message to application.");
Expand Down
15 changes: 9 additions & 6 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 @@ -269,10 +269,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;
Copy link
Member Author

@BrennanConroy BrennanConroy Jul 18, 2019

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 Expand Up @@ -347,8 +343,15 @@ private async Task StartSending(WebSocket socket)
{
if (WebSocketCanSend(socket))
{
// We're done sending, send the close frame to the client if the websocket is still open
await socket.CloseOutputAsync(error != null ? WebSocketCloseStatus.InternalServerError : WebSocketCloseStatus.NormalClosure, "", CancellationToken.None);
try
{
// We're done sending, send the close frame to the client if the websocket is still open
await socket.CloseOutputAsync(error != null ? WebSocketCloseStatus.InternalServerError : WebSocketCloseStatus.NormalClosure, "", CancellationToken.None);
}
catch (Exception ex)
{
Log.ClosingWebSocketFailed(_logger, ex);
}
}

_application.Input.Complete();
Expand Down
15 changes: 9 additions & 6 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 @@ -189,10 +189,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 Expand Up @@ -270,8 +266,15 @@ private async Task StartSending(WebSocket socket)
// Send the close frame before calling into user code
if (WebSocketCanSend(socket))
{
// We're done sending, send the close frame to the client if the websocket is still open
await socket.CloseOutputAsync(error != null ? WebSocketCloseStatus.InternalServerError : WebSocketCloseStatus.NormalClosure, "", CancellationToken.None);
try
{
// We're done sending, send the close frame to the client if the websocket is still open
await socket.CloseOutputAsync(error != null ? WebSocketCloseStatus.InternalServerError : WebSocketCloseStatus.NormalClosure, "", CancellationToken.None);
}
catch (Exception ex)
{
Log.ClosingWebSocketFailed(_logger, ex);
}
}

_application.Input.Complete();
Expand Down
8 changes: 8 additions & 0 deletions src/SignalR/common/Http.Connections/src/Internal/Transports/WebSocketsTransport.Log.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ private static class Log
private static readonly Action<ILogger, Exception> _closedPrematurely =
LoggerMessage.Define(LogLevel.Debug, new EventId(14, "ClosedPrematurely"), "Socket connection closed prematurely.");

private static readonly Action<ILogger, Exception> _closingWebSocketFailed =
LoggerMessage.Define(LogLevel.Debug, new EventId(15, "ClosingWebSocketFailed"), "Closing webSocket failed.");

public static void SocketOpened(ILogger logger, string subProtocol)
{
_socketOpened(logger, subProtocol, null);
Expand Down Expand Up @@ -122,6 +125,11 @@ public static void ClosedPrematurely(ILogger logger, Exception ex)
{
_closedPrematurely(logger, ex);
}

public static void ClosingWebSocketFailed(ILogger logger, Exception ex)
{
_closingWebSocketFailed(logger, ex);
}
}
}
}