-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Prevent WebSockets from throwing during graceful shutdown #26914
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http | |
/// </summary> | ||
internal sealed class Http1UpgradeMessageBody : Http1MessageBody | ||
{ | ||
private int _userCanceled; | ||
|
||
public Http1UpgradeMessageBody(Http1Connection context) | ||
: base(context) | ||
{ | ||
|
@@ -26,13 +28,13 @@ public Http1UpgradeMessageBody(Http1Connection context) | |
public override ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default) | ||
{ | ||
ThrowIfCompleted(); | ||
return _context.Input.ReadAsync(cancellationToken); | ||
return ReadAsyncInternal(cancellationToken); | ||
} | ||
|
||
public override bool TryRead(out ReadResult result) | ||
{ | ||
ThrowIfCompleted(); | ||
return _context.Input.TryRead(out result); | ||
return TryReadInternal(out result); | ||
} | ||
|
||
public override void AdvanceTo(SequencePosition consumed) | ||
|
@@ -54,6 +56,7 @@ public override void Complete(Exception exception) | |
|
||
public override void CancelPendingRead() | ||
{ | ||
Interlocked.Exchange(ref _userCanceled, 1); | ||
_context.Input.CancelPendingRead(); | ||
} | ||
|
||
|
@@ -69,12 +72,49 @@ public override Task StopAsync() | |
|
||
public override bool TryReadInternal(out ReadResult readResult) | ||
{ | ||
return _context.Input.TryRead(out readResult); | ||
// Ignore the canceled readResult unless it was canceled by the user. | ||
do | ||
{ | ||
if (!_context.Input.TryRead(out readResult)) | ||
{ | ||
return false; | ||
} | ||
} while (readResult.IsCanceled && Interlocked.Exchange(ref _userCanceled, 0) == 0); | ||
|
||
return true; | ||
} | ||
|
||
public override ValueTask<ReadResult> ReadAsyncInternal(CancellationToken cancellationToken = default) | ||
{ | ||
return _context.Input.ReadAsync(cancellationToken); | ||
ReadResult readResult; | ||
|
||
// Ignore the canceled readResult unless it was canceled by the user. | ||
do | ||
{ | ||
var readTask = _context.Input.ReadAsync(cancellationToken); | ||
|
||
if (!readTask.IsCompletedSuccessfully) | ||
{ | ||
return ReadAsyncInternalAwaited(readTask, cancellationToken); | ||
} | ||
|
||
readResult = readTask.GetAwaiter().GetResult(); | ||
} while (readResult.IsCanceled && Interlocked.Exchange(ref _userCanceled, 0) == 0); | ||
|
||
return new ValueTask<ReadResult>(readResult); | ||
} | ||
|
||
private async ValueTask<ReadResult> ReadAsyncInternalAwaited(ValueTask<ReadResult> readTask, CancellationToken cancellationToken = default) | ||
{ | ||
var readResult = await readTask; | ||
|
||
// Ignore the canceled readResult unless it was canceled by the user. | ||
while (readResult.IsCanceled && Interlocked.Exchange(ref _userCanceled, 0) == 0) | ||
{ | ||
readResult = await _context.Input.ReadAsync(cancellationToken); | ||
} | ||
|
||
return readResult; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stephentoub Is this acceptable ValueTask usage? I think we're okay because this always either awaits the ValueTask or calls GetAwaiter().GetResult(). And it only calls GetAwaiter().GetResult() if we've verified ValueTask.IsCompletedSuccessfully. I know we don't pool ValueTasks yet, but I don't want to introduce more bad code like what you highlighted in #16876. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should turn on the analyzer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks fine. And the analyzer shouldn't warn about it; if it does, we should fix the analysis. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,10 @@ | |
|
||
using System; | ||
using System.IO; | ||
using System.IO.Pipelines; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Connections; | ||
using Microsoft.AspNetCore.Connections.Features; | ||
using Microsoft.AspNetCore.Http.Features; | ||
using Microsoft.AspNetCore.Server.Kestrel.Core; | ||
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; | ||
|
@@ -343,5 +346,50 @@ await connection.Receive("HTTP/1.1 101 Switching Protocols", | |
await appCompletedTcs.Task.DefaultTimeout(); | ||
} | ||
} | ||
|
||
[Fact] | ||
public async Task DoesNotThrowGivenCanceledReadResult() | ||
{ | ||
var appCompletedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
||
await using var server = new TestServer(async context => | ||
{ | ||
try | ||
{ | ||
var upgradeFeature = context.Features.Get<IHttpUpgradeFeature>(); | ||
var duplexStream = await upgradeFeature.UpgradeAsync(); | ||
|
||
// Kestrel will call Transport.Input.CancelPendingRead() during shutdown so idle connections | ||
// can wake up and shutdown gracefully. We manually call CancelPendingRead() to simulate this and | ||
// ensure the Stream returned by UpgradeAsync doesn't throw in this case. | ||
// https://github.com/dotnet/aspnetcore/issues/26482 | ||
var connectionTransportFeature = context.Features.Get<IConnectionTransportFeature>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clever |
||
connectionTransportFeature.Transport.Input.CancelPendingRead(); | ||
|
||
// Use ReadAsync() instead of CopyToAsync() for this test since IsCanceled is only checked in | ||
// HttpRequestStream.ReadAsync() and not HttpRequestStream.CopyToAsync() | ||
Assert.Equal(0, await duplexStream.ReadAsync(new byte[1])); | ||
appCompletedTcs.SetResult(null); | ||
} | ||
catch (Exception ex) | ||
{ | ||
appCompletedTcs.SetException(ex); | ||
throw; | ||
} | ||
}, | ||
new TestServiceContext(LoggerFactory)); | ||
|
||
using (var connection = server.CreateConnection()) | ||
{ | ||
await connection.SendEmptyGetWithUpgrade(); | ||
await connection.Receive("HTTP/1.1 101 Switching Protocols", | ||
"Connection: Upgrade", | ||
$"Date: {server.Context.DateHeaderValue}", | ||
"", | ||
""); | ||
} | ||
|
||
await appCompletedTcs.Task.DefaultTimeout(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this end up in a tight loop until cancellation is consumed by the request processing loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if the PipeReader repeatedly returns a canceled ReadResult. Same goes ReadAsync. It should still be safe, because PipeReaders are supposed to only return at most 1 canceled ReadResult per call to CancelPendingRead(). And this logic isn't entirely new, even in 3.1. Http1ContentLengthMessageBody already has this behavior.
One difference you'll notice though between this and Http1ContentLengthMessageBody is that it doesn't call AdvanceTo before reading again after a canceled ReadResult (Remember #25799). This isn't necessary for the DefaultPipeReader and simplifies the logic quite a bit, but I could easily seeing this breaking custom ones. That said, if it breaks because of this, it would wind up just throwing an InvalidOperationException instead of the OperationCanceledException it throws today.
I understand being nervous though. Given severity of the issue (which is basically throwing from WebSockets 5 seconds earlier than we should during shutdown), do you still think this is worth backporting to 3.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should still back port it.