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

Clarify websocket activity failure #2325

Merged
merged 3 commits into from Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ReverseProxy/Forwarder/ForwarderError.cs
Expand Up @@ -103,4 +103,9 @@ public enum ForwarderError : int
/// Failed while creating the request message.
/// </summary>
RequestCreation,

/// <summary>
/// An upgraded request was idle and canceled due to the activity timeout.
/// </summary>
UpgradeActivityTimeout,
}
12 changes: 9 additions & 3 deletions src/ReverseProxy/Forwarder/HttpForwarder.cs
Expand Up @@ -745,9 +745,10 @@ private static ValueTask<bool> CopyResponseStatusAndHeadersAsync(HttpResponseMes
ForwarderError error;

var (firstResult, firstException) = await firstTask;
var activityTimedOut = activityCancellationSource.IsCancellationRequested && !activityCancellationSource.CancelledByLinkedToken;
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
if (firstResult != StreamCopyResult.Success)
{
error = ReportResult(context, requestFinishedFirst, firstResult, firstException);
error = ReportResult(context, requestFinishedFirst, firstResult, firstException, activityTimedOut);
// Cancel the other direction
activityCancellationSource.Cancel();
// Wait for this to finish before exiting so the resources get cleaned up properly.
Expand All @@ -765,7 +766,7 @@ private static ValueTask<bool> CopyResponseStatusAndHeadersAsync(HttpResponseMes
var (secondResult, secondException) = await secondTask;
if (!cancelReads && secondResult != StreamCopyResult.Success)
{
error = ReportResult(context, !requestFinishedFirst, secondResult, secondException!);
error = ReportResult(context, !requestFinishedFirst, secondResult, secondException!, activityTimedOut);
}
else
{
Expand All @@ -775,7 +776,7 @@ private static ValueTask<bool> CopyResponseStatusAndHeadersAsync(HttpResponseMes

return error;

ForwarderError ReportResult(HttpContext context, bool request, StreamCopyResult result, Exception exception)
ForwarderError ReportResult(HttpContext context, bool request, StreamCopyResult result, Exception exception, bool activityTimedOut)
{
var error = result switch
{
Expand All @@ -784,6 +785,10 @@ ForwarderError ReportResult(HttpContext context, bool request, StreamCopyResult
StreamCopyResult.Canceled => request ? ForwarderError.UpgradeRequestCanceled : ForwarderError.UpgradeResponseCanceled,
_ => throw new NotImplementedException(result.ToString()),
};
if (activityTimedOut)
{
error = ForwarderError.UpgradeActivityTimeout;
}
ReportProxyError(context, error, exception);
return error;
}
Expand Down Expand Up @@ -1039,6 +1044,7 @@ private static string GetMessage(ForwarderError error)
ForwarderError.UpgradeResponseCanceled => "Copying the upgraded response body was canceled.",
ForwarderError.UpgradeResponseClient => "The client reported an error when copying the upgraded response body.",
ForwarderError.UpgradeResponseDestination => "The destination reported an error when copying the upgraded response body.",
ForwarderError.UpgradeActivityTimeout => "The WebSocket connection was closed after being idle longer than the Activity Timeout.",
ForwarderError.NoAvailableDestinations => throw new NotImplementedException(), // Not used in this class
_ => throw new NotImplementedException(error.ToString()),
};
Expand Down
Expand Up @@ -10,4 +10,5 @@ internal enum WebSocketCloseReason : int
ServerGracefulClose,
ClientDisconnect,
ServerDisconnect,
ActivityTimeout,
}
Expand Up @@ -59,6 +59,9 @@ public WebSocketCloseReason GetCloseReason(HttpContext context)
ForwarderError.UpgradeRequestDestination => WebSocketCloseReason.ServerDisconnect,
ForwarderError.UpgradeResponseDestination => WebSocketCloseReason.ServerDisconnect,

// Activity Timeout
ForwarderError.UpgradeActivityTimeout => WebSocketCloseReason.ActivityTimeout,

// Both sides gracefully closed the underlying connection without sending a WebSocket close,
// or the server closed the connection and we canceled the client and suppressed the errors.
null => WebSocketCloseReason.ServerDisconnect,
Expand Down
4 changes: 1 addition & 3 deletions test/ReverseProxy.Tests/Forwarder/HttpForwarderTests.cs
Expand Up @@ -701,9 +701,7 @@ public async Task UpgradableRequest_CancelsIfIdle()

Assert.Equal(StatusCodes.Status101SwitchingProtocols, httpContext.Response.StatusCode);

// When both are idle it's a race which gets reported as canceled first.
Assert.True(ForwarderError.UpgradeRequestClient == result
|| ForwarderError.UpgradeResponseDestination == result);
Assert.Equal(ForwarderError.UpgradeActivityTimeout, result);

events.AssertContainProxyStages(upgrade: true);
}
Expand Down