[Stress] Enable POST by disabling InternalServerError suppression #58110

alnikola · 2021-08-25T13:25:43Z

Enable HTTP/2 POST stress tests because, the Kestrel's issue with an incorrect HTTP/2 stream resetting seems to be fixed by dotnet/aspnetcore#34768.

Contributes to #55261

Enable HTTP/2 POST stress tests because, the Kestrel's issue with an incorrect HTTP/2 stream resetting seems to be fixed by dotnet/aspnetcore#34768. Fixes #55261

ghost · 2021-08-25T13:25:52Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Enable HTTP/2 POST stress tests because, the Kestrel's issue with an incorrect HTTP/2 stream resetting seems to be fixed by dotnet/aspnetcore#34768.

Fixes #55261

Author:	alnikola
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs

antonfirsov · 2021-08-25T13:40:57Z

Don't we need to wait for the next aspnetcore preview for the fix to propagate? /cc @ManickaP

alnikola · 2021-08-25T14:13:07Z

/azp list

azure-pipelines · 2021-08-25T14:13:12Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress runtime-coreclr libraries-gcstress0x3-gcstress0xc runtime-coreclr libraries-gcstress-extra dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-richnav runtime-coreclr gc-largepages runtime-coreclr pgo runtime-coreclr libraries-pgo coreclr-gc-regions

alnikola · 2021-08-25T14:13:31Z

/azp run runtime-libraries stress-http

azure-pipelines · 2021-08-25T14:13:42Z

Azure Pipelines successfully started running 1 pipeline(s).

CarnaViire · 2021-08-25T14:20:26Z

Don't we need to wait for the next aspnetcore preview for the fix to propagate?

We might need to wait. Previous docker update was yesterday, but the ASP.NET Core fix might not be in yet. There is a PR for the next update dotnet/dotnet-docker#3073 but we also need to check whether SDK version is new enough.

CarnaViire · 2021-08-25T14:25:03Z

Hmm RC1 PR is not in yet dotnet/aspnetcore#35676 - I think docker uses pre-installed RC1 now... in today's stress run the version is Microsoft.AspNetCore.App/6.0.0-rc.1.21423.14

alnikola · 2021-08-25T14:27:54Z

Hmm RC1 PR is not in yet dotnet/aspnetcore#35676

Got it. I'll wait and the re-run the stress pipeline.

alnikola · 2021-08-26T10:48:51Z

/azp run runtime-libraries stress-http

azure-pipelines · 2021-08-26T10:49:00Z

Azure Pipelines successfully started running 1 pipeline(s).

alnikola · 2021-08-27T10:10:32Z

/azp run runtime-libraries stress-http

azure-pipelines · 2021-08-27T10:10:42Z

Azure Pipelines successfully started running 1 pipeline(s).

alnikola · 2021-08-27T13:07:15Z

/azp run runtime-libraries stress-http

azure-pipelines · 2021-08-27T13:08:20Z

Azure Pipelines successfully started running 1 pipeline(s).

alnikola · 2021-08-27T15:07:49Z

All HTTP/2 stress tests are green now. The fix is working. Please, do the final review and approve.

cc: @karelz

ManickaP · 2021-08-27T15:44:02Z

It's still there in HTTP/3 the same error. ~20 000 occurrences of:

System.Exception: Expected status code OK, got InternalServerError
    at HttpStress.ClientOperations.ValidateStatusCode(HttpResponseMessage m, HttpStatusCode expectedStatus) in /app/ClientOperations.cs:line 513
    at HttpStress.ClientOperations.<>c.<<get_Operations>b__1_10>d.MoveNext() in /app/ClientOperations.cs:line 446
 --- End of stack trace from previous location ---
    at HttpStress.StressClient.<>c__DisplayClass17_0.<<StartCore>g__RunWorker|0>d.MoveNext() in /app/StressClient.cs:line 204

https://dev.azure.com/dnceng/public/_build/results?buildId=1323480&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=1451f5f3-0108-5a08-5b92-e984b2a85bbd&l=140216

ManickaP · 2021-08-27T15:45:20Z

Should we reopen the kestrel issue or file another one? @karelz @JamesNK

alnikola · 2021-08-27T15:46:27Z

It's still there in HTTP/3 the same error. ~20 000 occurrences of:

@ManickaP Are you sure it's the same root cause? InternalServerError is just a generic 500 status, the reason can be completely different.

karelz · 2021-08-27T15:47:28Z

I assume the problem will be different in Kestrel for H/3.
Up to @JamesNK @Tratcher @adityamandaleeka if they want to create new issue or reuse existing one.

We will likely need to keep the test disabled -- but hopefully only for H/3 and not for H/2.

alnikola · 2021-08-27T15:48:55Z

I can add a protocol version check here, so the exception will be suppressed for H/3, but enabled for H/2.

ManickaP · 2021-08-27T15:50:45Z

We should keep either the original issue open or create a new one for H/3 and leave [ActiveIssue(...)] in the code.

I can add a protocol version check here, so the exception will be suppressed for H/3, but enabled for H/2.

Perfect!

ManickaP · 2021-08-27T15:53:21Z

We will likely need to keep the test disabled -- but hopefully only for H/3 and not for H/2.

Note that nothing was disabled per se, we just ignored certain type of errors because we knew that they're cause by a Kestrel bug.

antonfirsov · 2021-08-27T16:05:01Z

src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs

            {
                throw new Exception("IGNORE");
            }
-
+            


Nit: whitespaces:

Suggested change

It's done already :)

antonfirsov · 2021-08-27T16:06:53Z

#58110 (comment) vs ae8564d: telepathy alert!

alnikola · 2021-08-27T16:09:34Z

/azp run runtime-libraries stress-http

azure-pipelines · 2021-08-27T16:09:45Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2021-08-27T16:31:12Z

@alnikola we should backport this to release/6.0. (See #58272)

karelz · 2021-08-27T16:51:20Z

src/libraries/System.Net.Http/tests/StressTests/HttpStress/StressClient.cs

@@ -209,6 +209,11 @@ async Task RunWorker(int taskNum)
                    {
                        _aggregator.RecordCancellation(opIndex, stopwatch.Elapsed);
                    }
+                    catch (Exception e) when (e.Message == "IGNORE")


Don't we need version check here as well?

I believe - no, because this is a special exception thrown manually to suppress some errors. The throwing side does a version check, so it's redundant here.

Tratcher · 2021-08-27T16:54:45Z

What is the actual server error for HTTP/3? Let's find that before you open a new issue.

alnikola · 2021-08-30T10:32:27Z

I'm going to merge it with the link in the comments to the current issue #55261 which we can change it later.
Any objections?

ManickaP · 2021-08-31T15:23:21Z

@Tratcher It's POST ExpectContinue operation and the error is this:

2021-08-31 17:19:56.766 +02:00 [ERR] Connection id "0HMBCIB56DQAE", Request id "0HMBCIB56DQAE:00000018": An unhandled exception was thrown by the application.
System.NotImplementedException: The method or operation is not implemented.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3.Http3OutputProducer.Write100ContinueAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProduceContinueAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.TryProduceContinueAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.StartTimingReadAsync(ValueTask`1 readAwaitable, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3.Http3MessageBody.ReadAsync(CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1.StateMachineBox`1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token)
   at System.IO.Pipelines.PipeReader.CopyToAsyncCore[TStream](TStream destination, Func`4 writeAsync, CancellationToken cancellationToken)
   at HttpStress.StressServer.<>c.<<MapRoutes>b__6_6>d.MoveNext() in /home/manicka/repositories/runtime/src/libraries/System.Net.Http/tests/StressTests/HttpStress/StressServer.cs:line 249
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)

ManickaP · 2021-08-31T15:25:42Z

@alnikola @antonfirsov maybe we can just -xops 10 on the client side for H/3 instead of having hacks in the code... What do you think?

EDIT: I might do that in a separate PR if you want to get this in.

Tratcher · 2021-08-31T15:43:42Z

LOL, that's embarrassing... I filed dotnet/aspnetcore#35974 to implement 100-Continue. You'll need to skip that scenario until it's ready.

ManickaP · 2021-08-31T19:43:00Z

I put up a PR #58442

I removes the checks completely, making this one obsolete.

ManickaP · 2021-08-31T19:56:34Z

Original issue closed in #58442. We don't need the HTTP version check anymore.

[Stress] Enable POST by disabling InternalServerError suppression

cda0b74

Enable HTTP/2 POST stress tests because, the Kestrel's issue with an incorrect HTTP/2 stream resetting seems to be fixed by dotnet/aspnetcore#34768. Fixes #55261

alnikola requested a review from a team August 25, 2021 13:25

dotnet-issue-labeler bot added the area-System.Net.Http label Aug 25, 2021

antonfirsov reviewed Aug 25, 2021

View reviewed changes

src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs Show resolved Hide resolved

Remove catch side

b22674e

karelz approved these changes Aug 27, 2021

View reviewed changes

karelz requested a review from ManickaP August 27, 2021 15:47

alnikola added 3 commits August 27, 2021 18:00

Throw IGNORE for HTTP/2 only

faa0f34

Revert IGNORE catch

9467130

Remove whitespaces

ae8564d

antonfirsov reviewed Aug 27, 2021

View reviewed changes

antonfirsov approved these changes Aug 27, 2021

View reviewed changes

ManickaP approved these changes Aug 27, 2021

View reviewed changes

karelz approved these changes Aug 27, 2021

View reviewed changes

ManickaP mentioned this pull request Aug 31, 2021

[HTTP/3] Stress disable offending op causing server errors #58442

Merged

Tratcher mentioned this pull request Aug 31, 2021

Implement HTTP/3 100 Continue dotnet/aspnetcore#35974

Closed

ManickaP closed this Aug 31, 2021

karelz added this to the 7.0.0 milestone Aug 31, 2021

karelz mentioned this pull request Sep 2, 2021

[release/6.0] [HTTP/3] Stress disable offending op causing server errors #58541

Merged

jkotas deleted the alnikola/55261-enable-h2-stress-post branch September 18, 2021 04:11

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

[Stress] Enable POST by disabling InternalServerError suppression #58110

[Stress] Enable POST by disabling InternalServerError suppression #58110

Conversation

alnikola commented Aug 25, 2021 • edited by ManickaP Loading

ghost commented Aug 25, 2021

antonfirsov commented Aug 25, 2021

alnikola commented Aug 25, 2021

azure-pipelines bot commented Aug 25, 2021

alnikola commented Aug 25, 2021

azure-pipelines bot commented Aug 25, 2021

CarnaViire commented Aug 25, 2021

CarnaViire commented Aug 25, 2021

alnikola commented Aug 25, 2021

alnikola commented Aug 26, 2021

azure-pipelines bot commented Aug 26, 2021

alnikola commented Aug 27, 2021

azure-pipelines bot commented Aug 27, 2021

alnikola commented Aug 27, 2021

azure-pipelines bot commented Aug 27, 2021

alnikola commented Aug 27, 2021

ManickaP commented Aug 27, 2021

ManickaP commented Aug 27, 2021

alnikola commented Aug 27, 2021

karelz commented Aug 27, 2021

alnikola commented Aug 27, 2021

ManickaP commented Aug 27, 2021

ManickaP commented Aug 27, 2021

antonfirsov Aug 27, 2021

Choose a reason for hiding this comment

alnikola Aug 27, 2021

Choose a reason for hiding this comment

antonfirsov commented Aug 27, 2021

alnikola commented Aug 27, 2021

azure-pipelines bot commented Aug 27, 2021

antonfirsov commented Aug 27, 2021

karelz Aug 27, 2021

Choose a reason for hiding this comment

alnikola Aug 30, 2021

Choose a reason for hiding this comment

Tratcher commented Aug 27, 2021

alnikola commented Aug 30, 2021

ManickaP commented Aug 31, 2021

ManickaP commented Aug 31, 2021 • edited Loading

Tratcher commented Aug 31, 2021 • edited Loading

ManickaP commented Aug 31, 2021

ManickaP commented Aug 31, 2021

alnikola commented Aug 25, 2021 •

edited by ManickaP

Loading

ManickaP commented Aug 31, 2021 •

edited

Loading

Tratcher commented Aug 31, 2021 •

edited

Loading