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.

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

Fix flakyServerReset_AfterEndStream_NoError test #42030

Merged
merged 6 commits into from
Jun 17, 2022
Merged

Fix flakyServerReset_AfterEndStream_NoError test #42030

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f8638f3
Fix flakyServerReset_AfterEndStream_NoError test
halter73 Jun 3, 2022
5386635
Handle unlikely but possible FLOW_CONTROL_ERROR
halter73 Jun 3, 2022
2a0e63b
Simplify AppCompletedWithNoResponseBodyOrTrailers path
halter73 Jun 3, 2022
8ed549a
Apply suggestions from code review
halter73 Jun 17, 2022
3a17a4c
Add Debug.Assert
halter73 Jun 17, 2022
1a29075
Merge branch 'main' into halter73/41843
halter73 Jun 17, 2022
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
5 changes: 4 additions & 1 deletion AspNetCore.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MinimalJwtBearerSample", "s
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "dotnet-user-jwts.Tests", "src\Tools\dotnet-user-jwts\test\dotnet-user-jwts.Tests.csproj", "{89896261-C5DD-4901-BCA7-7A5F718BC008}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "stress", "stress", "{168F513D-56B1-4C7F-A73D-A738FCF8B51D}"
halter73 marked this conversation as resolved.
Show resolved Hide resolved
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -10940,7 +10942,7 @@ Global
{EE9D0952-6060-4723-B329-94A2950A6762} = {4FDDC525-4E60-4CAF-83A3-261C5B43721F}
{132D43A2-067A-4E24-A520-45B9F14DCB8E} = {EE9D0952-6060-4723-B329-94A2950A6762}
{2EC4E939-513F-44CD-A956-498966EAC929} = {7B976D8F-EA31-4C0B-97BD-DFD9B3CC86FB}
{987E1C29-F124-40C8-8E6F-1B2B6A4CB62A} = {4FDDC525-4E60-4CAF-83A3-261C5B43721F}
{987E1C29-F124-40C8-8E6F-1B2B6A4CB62A} = {168F513D-56B1-4C7F-A73D-A738FCF8B51D}
{3CBC4802-E9B8-48B7-BC8C-B0AFB9EEC643} = {0ACCEDA7-339C-4B4D-8DD4-1AC271F31C04}
{48E64014-B249-4644-8AEB-CDEE8ABA0DC2} = {3CBC4802-E9B8-48B7-BC8C-B0AFB9EEC643}
{1542DC58-1836-4191-A9C5-51D1716D2543} = {05A169C7-4F20-4516-B10A-B13C5649D346}
Expand Down Expand Up @@ -11154,6 +11156,7 @@ Global
{AB4B9E75-719C-4589-B852-20FBFD727730} = {0B200A66-B809-4ED3-A790-CB1C2E80975E}
{7F079E92-32D5-4257-B95B-CFFB0D49C160} = {7FD32066-C831-4E29-978C-9A2215E85C67}
{89896261-C5DD-4901-BCA7-7A5F718BC008} = {AB4B9E75-719C-4589-B852-20FBFD727730}
{168F513D-56B1-4C7F-A73D-A738FCF8B51D} = {4FDDC525-4E60-4CAF-83A3-261C5B43721F}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {3E8720B3-DBDD-498C-B383-2CC32A054E8F}
Expand Down
40 changes: 25 additions & 15 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,8 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St

FlushResult flushResult = default;

// There are 2 cases where we abort:
// 1. We're not complete but we got the abort.
// 2. We're complete and there's no more response data to be written.
if ((aborted && !completed) || (aborted && completed && actual == 0 && stream.ResponseTrailers is null or { Count: 0 }))
// We're not complete but we got the abort.
if (aborted && !completed)
{
// Response body is aborted, complete reader for this output producer.
if (flushHeaders)
Expand All @@ -176,9 +174,12 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
if (actual > 0)
{
// If we got here it means we're going to cancel the write. Restore any consumed bytes to the connection window.
lock (_windowUpdateLock)
if (actual > int.MaxValue || !TryUpdateConnectionWindow((int)actual))
halter73 marked this conversation as resolved.
Show resolved Hide resolved
{
_connectionWindow += actual;
// This branch can only ever be taken given both a buggy client and aborting streams mid-write. Even then, we're much more likely catch the
// error in Http2Connection.ProcessFrameAsync() before catching it here. This branch is technically possible though, so we defend against it.
await HandleFlowControlErrorAsync();
return;
}
}
}
Expand All @@ -196,6 +197,8 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
}
else if (completed && producer.AppCompletedWithNoResponseBodyOrTrailers)
{
Debug.Assert(flushHeaders, "The app completed successfully without flushing headers!");

if (buffer.Length != 0)
{
_log.Http2UnexpectedDataRemaining(stream.StreamId, _connectionId);
Expand All @@ -204,8 +207,7 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
{
stream.DecrementActiveClientStreamCount();

// Headers have already been written and there is no other content to write
flushResult = await FlushAsync(stream, flushHeaders, outputAborter: null, cancellationToken: default);
flushResult = await FlushEndOfStreamHeadersAsync(stream);
}
}
else
Expand Down Expand Up @@ -265,6 +267,18 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
_log.Http2ConnectionQueueProcessingCompleted(_connectionId);
}

private async Task HandleFlowControlErrorAsync()
{
var connectionError = new Http2ConnectionErrorException(CoreStrings.Http2ErrorWindowUpdateSizeInvalid, Http2ErrorCode.FLOW_CONTROL_ERROR);
_log.Http2ConnectionError(_connectionId, connectionError);
await WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.FLOW_CONTROL_ERROR);

// Prevent Abort() from writing an INTERNAL_ERROR GOAWAY frame after our FLOW_CONTROL_ERROR.
Complete();
// Stop processing any more requests and immediately close the connection.
_http2Connection.Abort(new ConnectionAbortedException(CoreStrings.Http2ErrorWindowUpdateSizeInvalid, connectionError));
}

private bool TryQueueProducerForConnectionWindowUpdate(long actual, Http2OutputProducer producer)
{
lock (_windowUpdateLock)
Expand Down Expand Up @@ -354,7 +368,7 @@ public void Abort(ConnectionAbortedException error)
}
}

private ValueTask<FlushResult> FlushAsync(Http2Stream stream, bool writeHeaders, IHttpOutputAborter? outputAborter, CancellationToken cancellationToken)
private ValueTask<FlushResult> FlushEndOfStreamHeadersAsync(Http2Stream stream)
{
lock (_writeLock)
{
Expand All @@ -363,16 +377,12 @@ private ValueTask<FlushResult> FlushAsync(Http2Stream stream, bool writeHeaders,
return default;
}

if (writeHeaders)
{
// write headers
WriteResponseHeadersUnsynchronized(stream.StreamId, stream.StatusCode, Http2HeadersFrameFlags.END_STREAM, (HttpResponseHeaders)stream.ResponseHeaders);
}
WriteResponseHeadersUnsynchronized(stream.StreamId, stream.StatusCode, Http2HeadersFrameFlags.END_STREAM, (HttpResponseHeaders)stream.ResponseHeaders);
halter73 marked this conversation as resolved.
Show resolved Hide resolved

var bytesWritten = _unflushedBytes;
_unflushedBytes = 0;

return _flusher.FlushAsync(_minResponseDataRate, bytesWritten, outputAborter, cancellationToken);
return _flusher.FlushAsync(_minResponseDataRate, bytesWritten);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,9 +436,10 @@ public ValueTask<FlushResult> WriteRstStreamAsync(Http2ErrorCode error)
{
lock (_dataWriterLock)
{
// Stop() always schedules a completion if one wasn't scheduled already.
Stop();
// We queued the stream to complete but didn't complete the response yet
if (_completeScheduled && !_completedResponse)
if (!_completedResponse)
{
// Set the error so that we can write the RST when the response completes.
_resetErrorCode = error;
Expand Down
1 change: 1 addition & 0 deletions src/Servers/Kestrel/Kestrel.slnf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"src\\Servers\\Kestrel\\samples\\SampleApp\\Kestrel.SampleApp.csproj",
"src\\Servers\\Kestrel\\samples\\SystemdTestApp\\SystemdTestApp.csproj",
"src\\Servers\\Kestrel\\samples\\http2cat\\http2cat.csproj",
"src\\Servers\\Kestrel\\stress\\HttpStress.csproj",
"src\\Servers\\Kestrel\\test\\InMemory.FunctionalTests\\InMemory.FunctionalTests.csproj",
"src\\Servers\\Kestrel\\test\\Interop.FunctionalTests\\Interop.FunctionalTests.csproj",
"src\\Servers\\Kestrel\\test\\Sockets.BindTests\\Sockets.BindTests.csproj",
Expand Down
102 changes: 58 additions & 44 deletions src/Servers/Kestrel/stress/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,54 @@ void ValidateContent(string expectedContent, string actualContent)
}
}

Func<ClientContext, Task> TestAbort(string path)
{
return async ctx =>
{
Version httpVersion = ctx.GetRandomVersion(httpVersions);
halter73 marked this conversation as resolved.
Show resolved Hide resolved
try
{
using (var req = new HttpRequestMessage(HttpMethod.Get, serverUri + path) { Version = httpVersion })
{
await ctx.HttpClient.SendAsync(req);
}
throw new Exception("Completed unexpectedly");
}
catch (Exception e)
{
if (e is HttpRequestException hre && hre.InnerException is IOException)
{
e = hre.InnerException;
}

if (e is IOException ioe)
{
if (httpVersion < HttpVersion.Version20)
{
return;
}

string name = e.InnerException?.GetType().Name;
halter73 marked this conversation as resolved.
Show resolved Hide resolved
switch (name)
{
case "Http2ProtocolException":
case "Http2ConnectionException":
case "Http2StreamException":
if (e.InnerException.Message.Contains("INTERNAL_ERROR") || e.InnerException.Message.Contains("CANCEL"))
halter73 marked this conversation as resolved.
Show resolved Hide resolved
{
return;
}
break;
case "WinHttpException":
return;
}
}

throw;
}
};
}

// Set of operations that the client can select from to run. Each item is a tuple of the operation's name
// and the delegate to invoke for it, provided with the HttpClient instance on which to make the call and
// returning asynchronously the retrieved response string from the server. Individual operations can be
Expand Down Expand Up @@ -181,51 +229,9 @@ void ValidateContent(string expectedContent, string actualContent)
}
}),

("GET Aborted",
async ctx =>
{
Version httpVersion = ctx.GetRandomVersion(httpVersions);
try
{
using (var req = new HttpRequestMessage(HttpMethod.Get, serverUri + "/abort") { Version = httpVersion })
{
await ctx.HttpClient.SendAsync(req);
}
throw new Exception("Completed unexpectedly");
}
catch (Exception e)
{
if (e is HttpRequestException hre && hre.InnerException is IOException)
{
e = hre.InnerException;
}

if (e is IOException ioe)
{
if (httpVersion < HttpVersion.Version20)
{
return;
}
("GET Abort", TestAbort("/abort")),

string name = e.InnerException?.GetType().Name;
switch (name)
{
case "Http2ProtocolException":
case "Http2ConnectionException":
case "Http2StreamException":
if (e.InnerException.Message.Contains("INTERNAL_ERROR") || e.InnerException.Message.Contains("CANCEL"))
{
return;
}
break;
case "WinHttpException":
return;
}
}

throw;
}
}),
("GET Parallel Abort", TestAbort("/parallel-abort")),

("POST",
async ctx =>
Expand Down Expand Up @@ -451,6 +457,14 @@ void ValidateContent(string expectedContent, string actualContent)
await context.Response.WriteAsync(contentSource.Substring(0, contentSource.Length / 2));
context.Abort();
});
endpoints.MapGet("/parallel-abort", async context =>
{
// Server writes some content and aborts the connection in the background.
Task writeTask = context.Response.WriteAsync(contentSource.Substring(0, contentSource.Length));
halter73 marked this conversation as resolved.
Show resolved Hide resolved
await Task.Yield();
context.Abort();
await writeTask;
});
endpoints.MapPost("/", async context =>
{
// Post echos back the requested content, first buffering it all server-side, then sending it all back.
Expand Down