Skip to content

Commit ab3e5e2

Browse files
halter73JamesNK
halter73
and
JamesNK
authored
Fix flakyServerReset_AfterEndStream_NoError test (#42030)
Co-authored-by: James Newton-King <[email protected]>
1 parent 92823e5 commit ab3e5e2

File tree

5 files changed

+95
-61
lines changed

5 files changed

+95
-61
lines changed

‎AspNetCore.sln

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1740,6 +1740,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Templates.Mvc.Tests", "src\
17401740
EndProject
17411741
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Templates.Blazor.Tests", "src\ProjectTemplates\test\Templates.Blazor.Tests\Templates.Blazor.Tests.csproj", "{281BF9DB-7B8A-446B-9611-10A60903F125}"
17421742
EndProject
1743+
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "stress", "stress", "{A5946454-4788-4871-8F23-A9471D55F115}"
1744+
EndProject
17431745
Global
17441746
GlobalSection(SolutionConfigurationPlatforms) = preSolution
17451747
Debug|Any CPU = Debug|Any CPU
@@ -11058,7 +11060,7 @@ Global
1105811060
{EE9D0952-6060-4723-B329-94A2950A6762} = {4FDDC525-4E60-4CAF-83A3-261C5B43721F}
1105911061
{132D43A2-067A-4E24-A520-45B9F14DCB8E} = {EE9D0952-6060-4723-B329-94A2950A6762}
1106011062
{2EC4E939-513F-44CD-A956-498966EAC929} = {7B976D8F-EA31-4C0B-97BD-DFD9B3CC86FB}
11061-
{987E1C29-F124-40C8-8E6F-1B2B6A4CB62A} = {4FDDC525-4E60-4CAF-83A3-261C5B43721F}
11063+
{987E1C29-F124-40C8-8E6F-1B2B6A4CB62A} = {A5946454-4788-4871-8F23-A9471D55F115}
1106211064
{3CBC4802-E9B8-48B7-BC8C-B0AFB9EEC643} = {0ACCEDA7-339C-4B4D-8DD4-1AC271F31C04}
1106311065
{48E64014-B249-4644-8AEB-CDEE8ABA0DC2} = {3CBC4802-E9B8-48B7-BC8C-B0AFB9EEC643}
1106411066
{1542DC58-1836-4191-A9C5-51D1716D2543} = {05A169C7-4F20-4516-B10A-B13C5649D346}
@@ -11280,6 +11282,7 @@ Global
1128011282
{F0FBA346-D8BC-4FAE-A4B2-85B33FA23055} = {08D53E58-4AAE-40C4-8497-63EC8664F304}
1128111283
{AA7445F5-BD28-400C-8507-E2E0D3CF7D7E} = {08D53E58-4AAE-40C4-8497-63EC8664F304}
1128211284
{281BF9DB-7B8A-446B-9611-10A60903F125} = {08D53E58-4AAE-40C4-8497-63EC8664F304}
11285+
{A5946454-4788-4871-8F23-A9471D55F115} = {4FDDC525-4E60-4CAF-83A3-261C5B43721F}
1128311286
EndGlobalSection
1128411287
GlobalSection(ExtensibilityGlobals) = postSolution
1128511288
SolutionGuid = {3E8720B3-DBDD-498C-B383-2CC32A054E8F}

‎src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,8 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
161161

162162
FlushResult flushResult = default;
163163

164-
// There are 2 cases where we abort:
165-
// 1. We're not complete but we got the abort.
166-
// 2. We're complete and there's no more response data to be written.
167-
if ((aborted && !completed) || (aborted && completed && actual == 0 && stream.ResponseTrailers is null or { Count: 0 }))
164+
// We're not complete but we got the abort.
165+
if (aborted && !completed)
168166
{
169167
// Response body is aborted, complete reader for this output producer.
170168
if (flushHeaders)
@@ -175,10 +173,17 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
175173

176174
if (actual > 0)
177175
{
176+
// actual > int.MaxValue should never be possible because it would exceed Http2PeerSettings.MaxWindowSize
177+
// which is a protocol-defined limit. There's no way Kestrel would try to write more than that in one go.
178+
Debug.Assert(actual <= int.MaxValue);
179+
178180
// If we got here it means we're going to cancel the write. Restore any consumed bytes to the connection window.
179-
lock (_windowUpdateLock)
181+
if (!TryUpdateConnectionWindow((int)actual))
180182
{
181-
_connectionWindow += actual;
183+
// 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
184+
// error in Http2Connection.ProcessFrameAsync() before catching it here. This branch is technically possible though, so we defend against it.
185+
await HandleFlowControlErrorAsync();
186+
return;
182187
}
183188
}
184189
}
@@ -196,6 +201,8 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
196201
}
197202
else if (completed && producer.AppCompletedWithNoResponseBodyOrTrailers)
198203
{
204+
Debug.Assert(flushHeaders, "The app completed successfully without flushing headers!");
205+
199206
if (buffer.Length != 0)
200207
{
201208
_log.Http2UnexpectedDataRemaining(stream.StreamId, _connectionId);
@@ -204,8 +211,7 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
204211
{
205212
stream.DecrementActiveClientStreamCount();
206213

207-
// Headers have already been written and there is no other content to write
208-
flushResult = await FlushAsync(stream, flushHeaders, outputAborter: null, cancellationToken: default);
214+
flushResult = await FlushEndOfStreamHeadersAsync(stream);
209215
}
210216
}
211217
else
@@ -265,6 +271,18 @@ static bool HasStateFlag(Http2OutputProducer.State state, Http2OutputProducer.St
265271
_log.Http2ConnectionQueueProcessingCompleted(_connectionId);
266272
}
267273

274+
private async Task HandleFlowControlErrorAsync()
275+
{
276+
var connectionError = new Http2ConnectionErrorException(CoreStrings.Http2ErrorWindowUpdateSizeInvalid, Http2ErrorCode.FLOW_CONTROL_ERROR);
277+
_log.Http2ConnectionError(_connectionId, connectionError);
278+
await WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.FLOW_CONTROL_ERROR);
279+
280+
// Prevent Abort() from writing an INTERNAL_ERROR GOAWAY frame after our FLOW_CONTROL_ERROR.
281+
Complete();
282+
// Stop processing any more requests and immediately close the connection.
283+
_http2Connection.Abort(new ConnectionAbortedException(CoreStrings.Http2ErrorWindowUpdateSizeInvalid, connectionError));
284+
}
285+
268286
private bool TryQueueProducerForConnectionWindowUpdate(long actual, Http2OutputProducer producer)
269287
{
270288
lock (_windowUpdateLock)
@@ -354,7 +372,7 @@ public void Abort(ConnectionAbortedException error)
354372
}
355373
}
356374

357-
private ValueTask<FlushResult> FlushAsync(Http2Stream stream, bool writeHeaders, IHttpOutputAborter? outputAborter, CancellationToken cancellationToken)
375+
private ValueTask<FlushResult> FlushEndOfStreamHeadersAsync(Http2Stream stream)
358376
{
359377
lock (_writeLock)
360378
{
@@ -363,16 +381,12 @@ private ValueTask<FlushResult> FlushAsync(Http2Stream stream, bool writeHeaders,
363381
return default;
364382
}
365383

366-
if (writeHeaders)
367-
{
368-
// write headers
369-
WriteResponseHeadersUnsynchronized(stream.StreamId, stream.StatusCode, Http2HeadersFrameFlags.END_STREAM, (HttpResponseHeaders)stream.ResponseHeaders);
370-
}
384+
WriteResponseHeadersUnsynchronized(stream.StreamId, stream.StatusCode, Http2HeadersFrameFlags.END_STREAM, (HttpResponseHeaders)stream.ResponseHeaders);
371385

372386
var bytesWritten = _unflushedBytes;
373387
_unflushedBytes = 0;
374388

375-
return _flusher.FlushAsync(_minResponseDataRate, bytesWritten, outputAborter, cancellationToken);
389+
return _flusher.FlushAsync(_minResponseDataRate, bytesWritten);
376390
}
377391
}
378392

‎src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,9 +436,10 @@ public ValueTask<FlushResult> WriteRstStreamAsync(Http2ErrorCode error)
436436
{
437437
lock (_dataWriterLock)
438438
{
439+
// Stop() always schedules a completion if one wasn't scheduled already.
439440
Stop();
440441
// We queued the stream to complete but didn't complete the response yet
441-
if (_completeScheduled && !_completedResponse)
442+
if (!_completedResponse)
442443
{
443444
// Set the error so that we can write the RST when the response completes.
444445
_resetErrorCode = error;

‎src/Servers/Kestrel/Kestrel.slnf

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
"src\\Servers\\Kestrel\\samples\\SampleApp\\Kestrel.SampleApp.csproj",
3232
"src\\Servers\\Kestrel\\samples\\SystemdTestApp\\SystemdTestApp.csproj",
3333
"src\\Servers\\Kestrel\\samples\\http2cat\\http2cat.csproj",
34+
"src\\Servers\\Kestrel\\stress\\HttpStress.csproj",
3435
"src\\Servers\\Kestrel\\test\\InMemory.FunctionalTests\\InMemory.FunctionalTests.csproj",
3536
"src\\Servers\\Kestrel\\test\\Interop.FunctionalTests\\Interop.FunctionalTests.csproj",
3637
"src\\Servers\\Kestrel\\test\\Sockets.BindTests\\Sockets.BindTests.csproj",

‎src/Servers/Kestrel/stress/Program.cs

Lines changed: 59 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,55 @@ void ValidateContent(string expectedContent, string actualContent)
112112
}
113113
}
114114

115+
Func<ClientContext, Task> TestAbort(string path)
116+
{
117+
return async ctx =>
118+
{
119+
var httpVersion = ctx.GetRandomVersion(httpVersions);
120+
try
121+
{
122+
using (var req = new HttpRequestMessage(HttpMethod.Get, serverUri + path) { Version = httpVersion })
123+
{
124+
await ctx.HttpClient.SendAsync(req);
125+
}
126+
throw new Exception("Completed unexpectedly");
127+
}
128+
catch (Exception e)
129+
{
130+
if (e is HttpRequestException hre && hre.InnerException is IOException)
131+
{
132+
e = hre.InnerException;
133+
}
134+
135+
if (e is IOException ioe)
136+
{
137+
if (httpVersion < HttpVersion.Version20)
138+
{
139+
return;
140+
}
141+
142+
var name = e.InnerException?.GetType().Name;
143+
switch (name)
144+
{
145+
case "Http2ProtocolException":
146+
case "Http2ConnectionException":
147+
case "Http2StreamException":
148+
// This can be improved when https://github.com/dotnet/runtime/issues/43239 is available.
149+
if (e.InnerException.Message.Contains("INTERNAL_ERROR") || e.InnerException.Message.Contains("CANCEL"))
150+
{
151+
return;
152+
}
153+
break;
154+
case "WinHttpException":
155+
return;
156+
}
157+
}
158+
159+
throw;
160+
}
161+
};
162+
}
163+
115164
// Set of operations that the client can select from to run. Each item is a tuple of the operation's name
116165
// and the delegate to invoke for it, provided with the HttpClient instance on which to make the call and
117166
// returning asynchronously the retrieved response string from the server. Individual operations can be
@@ -181,51 +230,9 @@ void ValidateContent(string expectedContent, string actualContent)
181230
}
182231
}),
183232

184-
("GET Aborted",
185-
async ctx =>
186-
{
187-
Version httpVersion = ctx.GetRandomVersion(httpVersions);
188-
try
189-
{
190-
using (var req = new HttpRequestMessage(HttpMethod.Get, serverUri + "/abort") { Version = httpVersion })
191-
{
192-
await ctx.HttpClient.SendAsync(req);
193-
}
194-
throw new Exception("Completed unexpectedly");
195-
}
196-
catch (Exception e)
197-
{
198-
if (e is HttpRequestException hre && hre.InnerException is IOException)
199-
{
200-
e = hre.InnerException;
201-
}
202-
203-
if (e is IOException ioe)
204-
{
205-
if (httpVersion < HttpVersion.Version20)
206-
{
207-
return;
208-
}
233+
("GET Abort", TestAbort("/abort")),
209234

210-
string name = e.InnerException?.GetType().Name;
211-
switch (name)
212-
{
213-
case "Http2ProtocolException":
214-
case "Http2ConnectionException":
215-
case "Http2StreamException":
216-
if (e.InnerException.Message.Contains("INTERNAL_ERROR") || e.InnerException.Message.Contains("CANCEL"))
217-
{
218-
return;
219-
}
220-
break;
221-
case "WinHttpException":
222-
return;
223-
}
224-
}
225-
226-
throw;
227-
}
228-
}),
235+
("GET Parallel Abort", TestAbort("/parallel-abort")),
229236

230237
("POST",
231238
async ctx =>
@@ -451,6 +458,14 @@ void ValidateContent(string expectedContent, string actualContent)
451458
await context.Response.WriteAsync(contentSource.Substring(0, contentSource.Length / 2));
452459
context.Abort();
453460
});
461+
endpoints.MapGet("/parallel-abort", async context =>
462+
{
463+
// Server writes some content and aborts the connection in the background.
464+
var writeTask = context.Response.WriteAsync(contentSource.Substring(0, contentSource.Length));
465+
await Task.Yield();
466+
context.Abort();
467+
await writeTask;
468+
});
454469
endpoints.MapPost("/", async context =>
455470
{
456471
// Post echos back the requested content, first buffering it all server-side, then sending it all back.

0 commit comments

Comments
 (0)