Skip to content

Commit 686da2e

Browse files
authored
Stop doing sync over async in Produce100Continue (#31650)
* Stop doing sync over async in Produce100Continue * Address feedback * IsCompleted -> IsCompletedSuccessfully * Remove unneeded awaited methods * Feedback * Brain fart * Update Http1ChunkedEncodingMessageBody.cs * Update Http2MessageBody.cs * Update Http2MessageBody.cs * Update Http1ContentLengthMessageBody.cs * Respond to feedback * Quarantine failling test
1 parent 1c6d9df commit 686da2e

File tree

8 files changed

+71
-33
lines changed

8 files changed

+71
-33
lines changed

src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami
4444

4545
public override bool TryReadInternal(out ReadResult readResult)
4646
{
47-
TryStart();
47+
TryStartAsync();
4848

4949
var boolResult = _requestBodyPipe.Reader.TryRead(out _readResult);
5050

@@ -61,7 +61,7 @@ public override bool TryReadInternal(out ReadResult readResult)
6161

6262
public override async ValueTask<ReadResult> ReadAsyncInternal(CancellationToken cancellationToken = default)
6363
{
64-
TryStart();
64+
await TryStartAsync();
6565

6666
try
6767
{
@@ -101,7 +101,7 @@ private async Task PumpAsync()
101101

102102
if (!awaitable.IsCompleted)
103103
{
104-
TryProduceContinue();
104+
await TryProduceContinueAsync();
105105
}
106106

107107
while (true)
@@ -171,7 +171,7 @@ protected override ValueTask OnStopAsync()
171171
// call complete here on the reader
172172
_requestBodyPipe.Reader.Complete();
173173

174-
Debug.Assert(_pumpTask != null, "OnReadStarted must have been called.");
174+
Debug.Assert(_pumpTask != null, "OnReadStartedAsync must have been called.");
175175

176176
// PumpTask catches all Exceptions internally.
177177
if (_pumpTask.IsCompleted)
@@ -195,9 +195,10 @@ private async ValueTask StopAsyncAwaited(Task pumpTask)
195195
_requestBodyPipe.Reset();
196196
}
197197

198-
protected override void OnReadStarted()
198+
protected override Task OnReadStartedAsync()
199199
{
200200
_pumpTask = PumpAsync();
201+
return Task.CompletedTask;
201202
}
202203

203204
private bool Read(ReadOnlySequence<byte> readableBuffer, PipeWriter writableBuffer, out SequencePosition consumed, out SequencePosition examined)

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public override async ValueTask<ReadResult> ReadAsyncInternal(CancellationToken
4545
KestrelBadHttpRequestException.Throw(RequestRejectionReason.RequestBodyTimeout);
4646
}
4747

48-
TryStart();
48+
await TryStartAsync();
4949

5050
// The while(true) loop is required because the Http1 connection calls CancelPendingRead to unblock
5151
// the call to StartTimingReadAsync to check if the request timed out.
@@ -132,7 +132,7 @@ public override bool TryReadInternal(out ReadResult readResult)
132132
KestrelBadHttpRequestException.Throw(RequestRejectionReason.RequestBodyTimeout);
133133
}
134134

135-
TryStart();
135+
TryStartAsync();
136136

137137
// The while(true) because we don't want to return a canceled ReadResult if the user themselves didn't cancel it.
138138
while (true)

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -909,27 +909,21 @@ protected bool VerifyResponseContentLength([NotNullWhen(false)] out Exception? e
909909
return true;
910910
}
911911

912-
public void ProduceContinue()
912+
public ValueTask<FlushResult> ProduceContinueAsync()
913913
{
914914
if (HasResponseStarted)
915915
{
916-
return;
916+
return default;
917917
}
918918

919919
if (_httpVersion != Http.HttpVersion.Http10 &&
920920
((IHeaderDictionary)HttpRequestHeaders).TryGetValue(HeaderNames.Expect, out var expect) &&
921921
(expect.FirstOrDefault() ?? "").Equals("100-continue", StringComparison.OrdinalIgnoreCase))
922922
{
923-
ValueTask<FlushResult> vt = Output.Write100ContinueAsync();
924-
if (vt.IsCompleted)
925-
{
926-
vt.GetAwaiter().GetResult();
927-
}
928-
else
929-
{
930-
vt.AsTask().GetAwaiter().GetResult();
931-
}
923+
return Output.Write100ContinueAsync();
932924
}
925+
926+
return default;
933927
}
934928

935929
public Task InitializeResponseAsync(int firstWriteByteCount)

src/Servers/Kestrel/Core/src/Internal/Http/IHttpResponseControl.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
1010
{
1111
internal interface IHttpResponseControl
1212
{
13-
void ProduceContinue();
13+
ValueTask<FlushResult> ProduceContinueAsync();
1414
Memory<byte> GetMemory(int sizeHint = 0);
1515
Span<byte> GetSpan(int sizeHint = 0);
1616
void Advance(int bytes);

src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,21 @@ public virtual ValueTask CompleteAsync(Exception? exception)
6666

6767
public virtual Task ConsumeAsync()
6868
{
69-
TryStart();
69+
Task startTask = TryStartAsync();
70+
if (!startTask.IsCompletedSuccessfully)
71+
{
72+
return ConsumeAwaited(startTask);
73+
}
7074

7175
return OnConsumeAsync();
7276
}
7377

78+
private async Task ConsumeAwaited(Task startTask)
79+
{
80+
await startTask;
81+
await OnConsumeAsync();
82+
}
83+
7484
public virtual ValueTask StopAsync()
7585
{
7686
TryStop();
@@ -93,20 +103,22 @@ public virtual void Reset()
93103
_examinedUnconsumedBytes = 0;
94104
}
95105

96-
protected void TryProduceContinue()
106+
protected ValueTask<FlushResult> TryProduceContinueAsync()
97107
{
98108
if (_send100Continue)
99109
{
100-
_context.HttpResponseControl.ProduceContinue();
101110
_send100Continue = false;
111+
return _context.HttpResponseControl.ProduceContinueAsync();
102112
}
113+
114+
return default;
103115
}
104116

105-
protected void TryStart()
117+
protected Task TryStartAsync()
106118
{
107119
if (_context.HasStartedConsumingRequestBody)
108120
{
109-
return;
121+
return Task.CompletedTask;
110122
}
111123

112124
OnReadStarting();
@@ -128,7 +140,7 @@ protected void TryStart()
128140
}
129141
}
130142

131-
OnReadStarted();
143+
return OnReadStartedAsync();
132144
}
133145

134146
protected void TryStop()
@@ -165,8 +177,9 @@ protected virtual void OnReadStarting()
165177
{
166178
}
167179

168-
protected virtual void OnReadStarted()
180+
protected virtual Task OnReadStartedAsync()
169181
{
182+
return Task.CompletedTask;
170183
}
171184

172185
protected void AddAndCheckObservedBytes(long observedBytes)
@@ -183,7 +196,15 @@ protected ValueTask<ReadResult> StartTimingReadAsync(ValueTask<ReadResult> readA
183196
{
184197
if (!readAwaitable.IsCompleted)
185198
{
186-
TryProduceContinue();
199+
ValueTask<FlushResult> continueTask = TryProduceContinueAsync();
200+
if (!continueTask.IsCompletedSuccessfully)
201+
{
202+
return StartTimingReadAwaited(continueTask, readAwaitable, cancellationToken);
203+
}
204+
else
205+
{
206+
continueTask.GetAwaiter().GetResult();
207+
}
187208

188209
if (_timingEnabled)
189210
{
@@ -195,6 +216,19 @@ protected ValueTask<ReadResult> StartTimingReadAsync(ValueTask<ReadResult> readA
195216
return readAwaitable;
196217
}
197218

219+
protected async ValueTask<ReadResult> StartTimingReadAwaited(ValueTask<FlushResult> continueTask, ValueTask<ReadResult> readAwaitable, CancellationToken cancellationToken)
220+
{
221+
await continueTask;
222+
223+
if (_timingEnabled)
224+
{
225+
_backpressure = true;
226+
_context.TimeoutControl.StartTimingRead();
227+
}
228+
229+
return await readAwaitable;
230+
}
231+
198232
protected void CountBytesRead(long bytesInReadResult)
199233
{
200234
var numFirstSeenBytes = bytesInReadResult - _alreadyTimedBytes;

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.Diagnostics;
56
using System.IO.Pipelines;
67
using System.Threading;
78
using System.Threading.Tasks;
89
using Microsoft.AspNetCore.Connections;
10+
using Microsoft.AspNetCore.Internal;
911
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
1012

1113
namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
@@ -30,13 +32,19 @@ protected override void OnReadStarting()
3032
}
3133
}
3234

33-
protected override void OnReadStarted()
35+
protected override Task OnReadStartedAsync()
3436
{
3537
// Produce 100-continue if no request body data for the stream has arrived yet.
3638
if (!_context.RequestBodyStarted)
3739
{
38-
TryProduceContinue();
40+
ValueTask<FlushResult> continueTask = TryProduceContinueAsync();
41+
if (!continueTask.IsCompletedSuccessfully)
42+
{
43+
return continueTask.GetAsTask();
44+
}
3945
}
46+
47+
return Task.CompletedTask;
4048
}
4149

4250
public override void Reset()
@@ -59,7 +67,7 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami
5967

6068
public override bool TryRead(out ReadResult readResult)
6169
{
62-
TryStart();
70+
TryStartAsync();
6371

6472
var hasResult = _context.RequestBodyPipe.Reader.TryRead(out readResult);
6573

@@ -80,7 +88,7 @@ public override bool TryRead(out ReadResult readResult)
8088

8189
public override async ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default)
8290
{
83-
TryStart();
91+
await TryStartAsync();
8492

8593
try
8694
{

src/Servers/Kestrel/Core/src/Internal/Http3/Http3MessageBody.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami
4646

4747
public override bool TryRead(out ReadResult readResult)
4848
{
49-
TryStart();
49+
TryStartAsync();
5050

5151
var hasResult = _context.RequestBodyPipe.Reader.TryRead(out readResult);
5252

@@ -67,7 +67,7 @@ public override bool TryRead(out ReadResult readResult)
6767

6868
public override async ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default)
6969
{
70-
TryStart();
70+
await TryStartAsync();
7171

7272
try
7373
{

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,7 @@ public async Task RemoveConnectionSpecificHeaders()
628628
}
629629

630630
[Fact]
631+
[QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/31777")]
631632
public async Task ContentLength_Received_NoDataFrames_Reset()
632633
{
633634
var headers = new[]

0 commit comments

Comments
 (0)