Stop doing sync over async in Produce100Continue #31650

wtgodbe · 2021-04-09T20:05:24Z

Draft with a first-pass at fixing the issue.

Tratcher · 2021-04-09T22:49:37Z

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

-            TryStart();
+            TryStartAsync();


Isn't this a behavior change to go from blocking to fire-and-forget? Can TryStartAsync throw asynchronously like if the client has disconnected?

If Write100ContinueAsync() could realistically return an incomplete ValueTask this is a change in behavior. Not sure how that could ever happen though. If it could happen, not blocking is waaay better than block because at least you return the thread to the pool. TryStartAsync should never be able to throw asynchronously because it's a write without a canceled token.

Add an assert. I dislike the fire and forget as well unless we have an explicit check that it's always completed.

Whoops, plus a10c9d9

I love that bug 😄

I dislike the fire and forget

I totally agree. I also dislike fire-and-forget with a passion, but what's even worse is blocking on I/O. This isn't the only place we don't await output-writing ValueTasks we expect to be usually completed.

Now unlike with HTTP/2 where you could conceive of a scenario where output backpressure has built up prior to sending a WINDOW_UPDATE or RST_STREAM, it's hard to come up with one for an HTTP/1.1 "100 continue" response. That said, it's not a mathematical invariant. I could write a unit test with a custom transport and write buffering disabled to force it. For that reason, I don't think we should add this Debug.Assert like we don't for WINDOW_UPDATE or RST_STREAM. If we think it's worthwhile we can store the task and await it in ConsumeAsync(), but I don't think that's necessary.

That makes sense to me - if it matches what we do for RST_STREAM and WINDOW_UPDATE seems like we can remove the assert here

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

wtgodbe · 2021-04-12T20:32:20Z

Got what looks to be a flaky failure in a selenium test, InputDateInteractsWithEditContext_NonNullableDateTime. CC @captainsafia since you're on build ops

Other than that, any other concerns @Tratcher @davidfowl?

OpenQA.Selenium.BrowserAssertFailedException : Xunit.Sdk.EqualException: Assert.Equal() Failure\r\nExpected: modified valid\r\nActual: modified invalid\r\n at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer1 comparer) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\EqualityAsserts.cs:line 40\r\n at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass17_0.b__0() in /_/src/Shared/E2ETesting/WaitAssert.cs:line 83\r\n at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass18_01.b__0(IWebDriver ) in //src/Shared/E2ETesting/WaitAssert.cs:line 109\r\nScreen shot captured at 'D:\workspace_work\1\s\artifacts\TestResults\Release\Microsoft.AspNetCore.Components.E2ETests\bf12e230a077482c93a44465ccda36f2.png'\r\nEncountered browser errors\r\n[2021-04-12T17:01:25Z] [Info] http://127.0.0.1:53621/subdir/_framework/blazor.server.js 0:31687 "[2021-04-12T17:01:25.992Z] Information: Normalizing '_blazor' to 'http://127.0.0.1:53621/subdir/_blazor'."\r\n[2021-04-12T17:01:25Z] [Info] http://127.0.0.1:53621/subdir 45:16 "Blazor server-side"\r\n[2021-04-12T17:01:26Z] [Info] http://127.0.0.1:53621/subdir/_framework/blazor.server.js 0:31687 "[2021-04-12T17:01:25.998Z] Information: WebSocket connected to ws://127.0.0.1:53621/subdir/_blazor?id=foJb3ozHB7bXxtdKAJ8Kbg."\r\n[2021-04-12T17:01:27Z] [Info] http://127.0.0.1:53621/subdir/_framework/blazor.server.js 0:31687 "[2021-04-12T17:01:27.398Z] Information: Normalizing '_blazor' to 'http://127.0.0.1:53621/subdir/_blazor'."\r\n[2021-04-12T17:01:27Z] [Info] http://127.0.0.1:53621/subdir 45:16 "Blazor server-side"\r\n[2021-04-12T17:01:27Z] [Info] http://127.0.0.1:53621/subdir/_framework/blazor.server.js 0:31687 "[2021-04-12T17:01:27.408Z] Information: WebSocket connected to ws://127.0.0.1:53621/subdir/_blazor?id=n_74YY7aNbRQ53NAUSdGuQ."Page content:\r\n\r\n \r\n <title>Basic test app</title>\r\n \r\n \r\n \r\n\r\n \r\n \r\n \r\n\r\n\r\n
\r\n Select test:\r\n Choose...\r\n After-render interop component\r\n Auth cases\r\n bind cases\r\n Component ref component\r\n

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

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

JamesNK · 2021-04-13T00:18:20Z

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

+            Task readTask = OnReadStartedAsync();
+            if (!readTask.IsCompletedSuccessfully)
+            {
+                return readTask;
+            }
+
+            return Task.CompletedTask;


Why not:

Suggested change

Task readTask = OnReadStartedAsync();

if (!readTask.IsCompletedSuccessfully)

{

return readTask;

}

return Task.CompletedTask;

return OnReadStartedAsync();

If the OnReadStartedAsync task is complete then return just return it. And if it is not complete then also just return it...

JamesNK · 2021-04-13T00:22:40Z

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

@@ -183,7 +202,11 @@ protected ValueTask<ReadResult> StartTimingReadAsync(ValueTask<ReadResult> readA
        {
            if (!readAwaitable.IsCompleted)
            {
-                TryProduceContinue();
+                ValueTask<FlushResult> continueTask = TryProduceContinueAsync();


Shouldn't we indicate that the ValueTask result has been consumed? @stephentoub submitted a PR to do that for historical code: #31221

if (!continueTask.IsCompletedSuccessfully) { return StartTimingReadAwaited(continueTask, readAwaitable, cancellationToken); } else { continueTask.GetAwaiter().GetResult(); }

Hmm. I'd have expected the code in the PR to trigger CA2012, providing the same feedback as James. Were no warnings issued for this?

Your .editconfig changes are still there: https://github.com/dotnet/aspnetcore/blob/main/.editorconfig

I'm not familiar with configuring analyzers in editconfigs or CA2021. I'm not sure why it wasn't prompted.

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

wtgodbe · 2021-04-13T17:00:55Z

@JamesNK @halter73 just pushed a9a5619 to respond to feedback

wtgodbe · 2021-04-13T19:44:36Z

Got a real test failure in Microsoft.AspNetCore.Server.Kestrel.Core.Tests.Http3StreamTests.ContentLength_Received_NoDataFrames_Reset

Assert.Equal() Failure
Expected: ProtocolError
Actual: 0

Looking

jkotalik · 2021-04-13T19:46:09Z

I think it isn't you: #31743 (comment)

jkotalik · 2021-04-13T19:46:23Z

@JamesNK should we just disable in this PR?

JamesNK · 2021-04-13T19:49:17Z

Issue: #31777

Add skip or quarantine back in this PR.

wtgodbe · 2021-04-13T19:56:52Z

Done

wtgodbe · 2021-04-13T20:06:52Z

@JamesNK any other concerns?

Stop doing sync over async in Produce100Continue

5156206

wtgodbe requested a review from halter73 April 9, 2021 20:05

ghost added the area-runtime label Apr 9, 2021

wtgodbe added 3 commits April 9, 2021 14:40

Address feedback

5c25fa2

IsCompleted -> IsCompletedSuccessfully

0503f75

Remove unneeded awaited methods

8d8a146

wtgodbe marked this pull request as ready for review April 9, 2021 22:40

wtgodbe requested review from jkotalik and Tratcher as code owners April 9, 2021 22:40

Tratcher reviewed Apr 9, 2021

View reviewed changes

halter73 reviewed Apr 9, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs Outdated Show resolved Hide resolved

halter73 approved these changes Apr 9, 2021

View reviewed changes

wtgodbe added 2 commits April 12, 2021 09:27

Feedback

70e8ee1

Brain fart

a10c9d9

Tratcher approved these changes Apr 12, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs Show resolved Hide resolved

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs Show resolved Hide resolved

jkotalik approved these changes Apr 12, 2021

View reviewed changes

wtgodbe added 3 commits April 12, 2021 14:41

Update Http1ChunkedEncodingMessageBody.cs

f91acea

Update Http2MessageBody.cs

b93be28

Update Http2MessageBody.cs

a99aea8

davidfowl approved these changes Apr 12, 2021

View reviewed changes

Update Http1ContentLengthMessageBody.cs

b147ac7

JamesNK requested changes Apr 13, 2021

View reviewed changes

Respond to feedback

a9a5619

Merge remote-tracking branch 'origin/main' into wtgodbe/100Continue

361f009

Quarantine failling test

7893fbd

JamesNK approved these changes Apr 13, 2021

View reviewed changes

wtgodbe merged commit 686da2e into main Apr 13, 2021

wtgodbe deleted the wtgodbe/100Continue branch April 13, 2021 22:27

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop doing sync over async in Produce100Continue #31650

Stop doing sync over async in Produce100Continue #31650

wtgodbe commented Apr 9, 2021

Tratcher Apr 9, 2021

halter73 Apr 9, 2021

davidfowl Apr 10, 2021

wtgodbe Apr 12, 2021

wtgodbe Apr 12, 2021

davidfowl Apr 12, 2021

halter73 Apr 12, 2021 •

edited

Loading

wtgodbe Apr 12, 2021

wtgodbe commented Apr 12, 2021 •

edited

Loading

JamesNK Apr 13, 2021

JamesNK Apr 13, 2021

stephentoub Apr 13, 2021

JamesNK Apr 13, 2021

wtgodbe commented Apr 13, 2021

wtgodbe commented Apr 13, 2021

jkotalik commented Apr 13, 2021

jkotalik commented Apr 13, 2021

JamesNK commented Apr 13, 2021

wtgodbe commented Apr 13, 2021

wtgodbe commented Apr 13, 2021

Stop doing sync over async in Produce100Continue #31650

Stop doing sync over async in Produce100Continue #31650

Conversation

wtgodbe commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgodbe commented Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgodbe commented Apr 13, 2021

wtgodbe commented Apr 13, 2021

jkotalik commented Apr 13, 2021

jkotalik commented Apr 13, 2021

JamesNK commented Apr 13, 2021

wtgodbe commented Apr 13, 2021

wtgodbe commented Apr 13, 2021

halter73 Apr 12, 2021 •

edited

Loading

wtgodbe commented Apr 12, 2021 •

edited

Loading