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

Decreasing the window size below the amount of data sent can prematurely abort the connection #47452

Closed
BrennanConroy opened this issue Mar 27, 2023 · 3 comments · Fixed by #58575
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-kestrel
Milestone

Comments

@BrennanConroy
Copy link
Member

The following error can occur in specific scenarios if the server has sent some data on a stream and the client updates the window size to some value below the amount already sent

Microsoft.AspNetCore.Server.Kestrel.Http2 Critical: The event loop in connection 0HMP8EQDO1NJO failed unexpectedly.
System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'length')
    at System.ThrowHelper.ThrowStartOrEndArgumentValidationException(Int64 start)
    at System.Buffers.ReadOnlySequence`1.Slice(Int64 start, Int64 length)
    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.WriteToOutputPipe() in /_/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs:line 139

Method 1

For example, if the server sends 2 bytes with an initial window size of 3, then the client updates the window to be 1. The server will successfully have scheduled a new item to the output loop for processing:

if (_streamWindow > 0)
{
Schedule();

But before it starts processing the item in
private async Task WriteToOutputPipe()
{
while (await _channel.Reader.WaitToReadAsync())
{
// We need to handle the case where aborts can be scheduled while this loop is running and might be on the way to complete
// the reader.
while (_channel.Reader.TryRead(out var producer) && !producer.CompletedResponse)

The updated Window can be processed:
var windowSizeDifference = (int)_clientSettings.InitialWindowSize - previousInitialWindowSize;
if (windowSizeDifference != 0)
{
foreach (var stream in _streams.Values)
{
if (!stream.TryUpdateOutputWindow(windowSizeDifference))

Which will decrease the _streamWindow to a negative value, which will then fail the buffer slice at
var actual = producer.CheckStreamWindow(buffer.Length);
// Now check the connection window
actual = CheckConnectionWindow(actual);
// Write what we can
if (actual < buffer.Length)
{
buffer = buffer.Slice(0, actual);


Method 2

Another way this can be hit is if Stop is called once the _streamWindow is negative, even if nothing has been scheduled yet, because it will call Schedule itself without any checks for _streamWindow

public void Stop()
{
lock (_dataWriterLock)
{
_waitingForWindowUpdates = false;
if (_completeScheduled && _completedResponse)
{
// We can overschedule as long as we haven't yet completed the response. This is important because
// we may need to abort the stream if it's waiting for a window update.
return;
}
_completeScheduled = true;
EnqueueStateUpdate(State.Aborted);
Schedule();


Method 3

Doing multiple writes can also cause this, if the window update happens after the first write, but before the second one (or if it happens during the race between starting to process the second write and reading the settings as described in method 1).
This path also calls Schedule without a check for _streamWindow.

lock (_dataWriterLock)
{
ThrowIfSuffixSentOrCompleted();
// This length check is important because we don't want to set _startedWritingDataFrames unless a data
// frame will actually be written causing the headers to be flushed.
if (_completeScheduled || data.Length == 0)
{
return Task.CompletedTask;
}
_startedWritingDataFrames = true;
_pipeWriter.Write(data);
EnqueueDataWrite(data.Length);
var task = _flusher.FlushAsync(this, cancellationToken).GetAsTask();
Schedule();


This following test will hit the issue in either the Stop path described in method 2 or the multi write path described in method 3.

[Fact]
public async Task DecreaseWindowBelowCurrentSentAmount()
{
    const int windowSize = 3;
    _clientSettings.InitialWindowSize = windowSize;

    var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);

    await InitializeConnectionAsync(async context =>
    {
        var bodyControlFeature = context.Features.Get<IHttpBodyControlFeature>();
        bodyControlFeature.AllowSynchronousIO = true;
        await context.Response.Body.WriteAsync(new byte[windowSize - 1], 0, windowSize - 1);
        await tcs.Task;
        context.Response.Body.Write(new byte[windowSize], 0, windowSize);
    });

    await StartStreamAsync(1, _browserRequestHeaders, endStream: true);

    await ExpectAsync(Http2FrameType.HEADERS,
        withLength: 32,
        withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS,
        withStreamId: 1);

    // Decrease window size after server has already sent the current window - 1 size of data
    _clientSettings.InitialWindowSize = windowSize - 2;
    await SendSettingsAsync();

    await ExpectAsync(Http2FrameType.DATA,
        withLength: windowSize - 1,
        withFlags: (byte)Http2DataFrameFlags.NONE,
        withStreamId: 1);

    await ExpectAsync(Http2FrameType.SETTINGS,
        withLength: 0,
        withFlags: (byte)Http2DataFrameFlags.END_STREAM,
        withStreamId: 0);

    tcs.SetResult();

    await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false);
}
@BrennanConroy BrennanConroy added bug This issue describes a behavior which is not expected - a bug. area-runtime feature-kestrel labels Mar 27, 2023
@BrennanConroy BrennanConroy added this to the Backlog milestone Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@BrennanConroy BrennanConroy removed their assignment Apr 6, 2023
@BrennanConroy
Copy link
Member Author

Moving to backlog. This is a low-pri bug, only affects an edge case in the HTTP/2 spec and only sometimes since it's a race.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@bhaskarqlik
Copy link
Contributor

We are encountering this issue intermittently after we upgraded our service from .NET 6 to 8.

crit: Microsoft.AspNetCore.Server.Kestrel.Http2[63]
      The event loop in connection 0HN3R40QVJ560 failed unexpectedly.
      System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'length')
         at System.ThrowHelper.ThrowStartOrEndArgumentValidationException(Int64 start)
         at System.Buffers.ReadOnlySequence`1.Slice(Int64 start, Int64 length)
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.WriteToOutputPipe()

Our service is a grpc server that provides streaming calls to transfer large volumes of data. The grpc client in our case is written in C++. After the error, the grpc client is not notified of any errors, and it remains waiting for a response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-kestrel
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@BrennanConroy @amcasey @wtgodbe @bhaskarqlik and others