[HTTP/3] Stress hack for msquic dropping connections #84793

ManickaP · 2023-04-13T17:18:55Z

Implement the same hack as for functional tests to prevent msquic from dropping connections, see #83687.

Fixes #77126

…m dropping connections

ghost · 2023-04-13T17:19:10Z

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

Issue Details

Implement the same hack as for functional tests to prevent msquic from dropping connections, see #83687.

Fixes #77126

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

ManickaP · 2023-04-13T17:19:46Z

/azp run runtime-libraries stress-http

azure-pipelines · 2023-04-13T17:19:56Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2023-04-13T18:17:00Z

src/libraries/System.Net.Http/tests/StressTests/HttpStress/HttpStress.csproj

+    <Compile Include="..\..\..\..\Common\src\System\Net\Logging\NetEventSource.Common.cs" Link="Common\System\Net\Logging\NetEventSource.Common.cs" />
+    <Compile Include="..\..\..\..\System.Net.Quic\src\System\Net\Quic\Internal\MsQuicApi.cs" Link="ProductionCode\System\Net\Quic\Internal\MsQuicApi.cs" />
+    <Compile Include="..\..\..\..\System.Net.Quic\src\System\Net\Quic\Internal\MsQuicSafeHandle.cs" Link="ProductionCode\System\Net\Quic\Internal\MsQuicSafeHandle.cs" />
+    <Compile Include="..\..\..\..\System.Net.Quic\src\System\Net\Quic\Interop\*.cs" Link="ProductionCode\System\Net\Quic\Interop\*.cs" />
+    <Compile Include="..\..\..\..\System.Net.Quic\src\System\Net\Quic\NetEventSource.Quic.cs" Link="ProductionCode\System\Net\Quic\NetEventSource.Quic.cs" />


A refactor of these internals may break stress builds without us noticing, ~~but I guess there is no good way to deal with this. We should remember that HttpStress took a dependency.~~

Also: these files are missing from the docker context on Windows, which is why the Windows build failed. Copying them would make this solution even more fragile. Can we implement the worker delay tuning hack with reflection instead and share that implementation between System.Net.Quic.Functional.Tests and the stress tests to make sure we notice if it's broken? That would also remove the need of including anything but QuicDefaults.cs in System.Net.Quic.Functional.Tests.csproj (apart from the utility containing the hack).

Ah no, we would need #75347 to invoke function pointers with reflection safely. I guess, the best would be to copy & include msquic_generated.cs, then do a combination of reflection + code that only depends on the generated types.

I can take over if you want.

Reflection was one of the options we discussed with @CarnaViire when implementing this in functional tests. I was able to workaround it in functional tests with source code inclusion, which is normal for corresponding tests. However, here it proves problematic.
I could re-work this to create an internal function on MsQuicApi which we would invoke via reflection from both functional tests as well as stress.

ManickaP · 2023-04-14T09:30:09Z

/azp run runtime-libraries stress-http

azure-pipelines · 2023-04-14T09:30:24Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2023-04-14T11:57:19Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

+    // Do not change the name and signature without looking for textual occurrences!
+    // This method is invoked via reflection from QUIC functional and HTTP stress tests.
+    private static (bool, string) SetUpForTests()


This is definitely the easiest option to solve this problem, but I thought including test-only methods in production assemblies is no-go. Do we have any existing precedent in the BCL?

I haven't thought about that, but I found this for example:

runtime/src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackDecoder.cs

Lines 109 to 110 in 2d833f4

// For testing.

internal HPackDecoder(int maxDynamicTableSize, int maxHeadersLength, DynamicTable dynamicTable)

I can always go back to code sharing though and try to minimize the number of shared files, either with some some reflection, strategically placed #if and/or code stubs.

The HPack ctr is not "dead code" otherwise, because it's also being used by the public constructor above. For me having a private, test-only method is a good tradeoff, considering the maintenance burden that comes with other options, but we should stick to BCL guidance. @stephentoub is it ok to expose such methods?

Combo of shared code + reflection is there. @antonfirsov could you look into copying the appropriate files in the docker container please?

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

… some reflection

wfurt · 2023-04-21T21:27:03Z

eng/docker/libraries-sdk.linux.Dockerfile

@@ -1,5 +1,5 @@
 # Builds and copies library artifacts into target dotnet sdk image
-ARG BUILD_BASE_IMAGE=mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-f39df28-20191023143754
+ARG BUILD_BASE_IMAGE=mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7


8.0 will not support Centos 7 AFAIK (hurray!) we should update this at some point

Stream9??? Or should we swap for different distro?

Is there a reason we are running stress tests on Centos and not ubuntu, unlike enterprise tests?

the centos-7 images have build tools and we use them for product. I don't know if that is needed.
I think we should pick whatever is easiest for us to maintain.

I'm switching to centos-stream8 in #85342, which seems to work.

wfurt · 2023-04-21T21:34:46Z

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs

@@ -44,14 +44,20 @@ public abstract class QuicTestBase : IDisposable

        static unsafe QuicTestBase()
        {
-            Console.WriteLine($"MsQuic {(IsSupported ? "supported" : "not supported")} and using '{MsQuicApi.MsQuicLibraryVersion}'.");
+            // If any of the reflection bellow breaks due to changes in "System.Net.Quic.MsQuicApi", also check and fix HttpStress project as it uses the same hack.


I'm wondering if this should go somewhere to Common/test/.... so we can avoid duplication.
It may be worth of adding explanation as this is unusual craft.

Sounds good, I'll look into it.

We should keep in mind that whatever utility helper we add, its' file has to be also copied to the container, so avoiding duplication has a higher cost in complexity here than normally. At the moment I'm undecided if it's worth it, would prefer to do the docker copying work mentioned in #84793 (comment) first, planning to jump to it tomorrow.

Ok, I'm holding back. Let me know when/if I should look into it.

antonfirsov · 2023-04-26T15:00:49Z

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

@@ -158,6 +162,9 @@ private static async Task<ExitCode> Run(Configuration config)

            string GetAssemblyInfo(Assembly assembly) => $"{assembly.Location}, modified {new FileInfo(assembly.Location).LastWriteTime}";

+            Type msQuicApiType = typeof(QuicConnection).Assembly.GetType("System.Net.Quic.MsQuicApi");
+            string msQuicLibraryVersion = (string)msQuicApiType.GetProperty("MsQuicLibraryVersion", BindingFlags.NonPublic | BindingFlags.Static).GetGetMethod(true).Invoke(null, Array.Empty<object?>());


@ManickaP unfortunately, this line fails with NullReferenceException in release builds. I assume the MsQuicLibraryVersion is being trimmed away, since it's unused in the production code.

We could workaround this by moving the version extraction to a shared test-utility (together with the MaxWorkerQueue hack helper), but IMHO it's not worth it unless we think that it's super-important to know the msquic version for stress test troubleshooting.

For now I'm just removing it.

is it possible that there is image without MsQuic?

What do you mean? If you are suspecting lack of msquic as the root cause of this error - it's not the case. Removing code messing with MsQuicLibraryVersion makes things work (= HTTP/3 stress runs).

is it possible that there is image without MsQuic?

In that case you'd get "unknown" as the MsQuic version, the code expects that.

Another option would be to tell the trimmer to keep it there via DynamicDependency. And this is just one string.

We should try to clarify whether test-only usage of DynamicDependency is ok, without that, I prefer to defensively assume it's not.

… avoid the property being trimmed

antonfirsov · 2023-04-27T13:10:32Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

@@ -143,7 +143,7 @@ static MsQuicApi()

            if (NetEventSource.Log.IsEnabled())
            {
-                NetEventSource.Info(null, $"Loaded MsQuic library version '{version}', commit '{gitHash}'.");
+                NetEventSource.Info(null, $"Loaded MsQuic library '{MsQuicLibraryVersion}'.");


Brilliant 😆

ManickaP · 2023-04-27T14:57:34Z

If this passes, I think it's done. Thanks for the help @antonfirsov !

ManickaP · 2023-04-27T15:07:03Z

I'm seeing this build error in Linux runs:

/app/Directory.Build.targets(2,3): error MSB4019: The imported project "/live-runtime-artifacts/targetingpacks.targets" was not found. Confirm that the expression in the Import declaration "/live-runtime-artifacts/targetingpacks.targets" is correct, and that the file exists on disk. [/app/HttpStress.csproj]

Anything familiar Anton?

antonfirsov · 2023-04-27T15:51:30Z

Anything familiar Anton?

This failed in multiple runs on this PR, and not on main, need to check if my changes broke it.

antonfirsov · 2023-04-27T18:32:06Z

/azp run runtime-libraries stress-http

azure-pipelines · 2023-04-27T18:32:14Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov

This is good now. Fixed the linux build with 949d82b. The failure in the last run is #76183.

Implement the same hack as for functional tests to prevent msquic fro…

df45d59

…m dropping connections

dotnet-issue-labeler bot added the area-System.Net.Http label Apr 13, 2023

ghost assigned ManickaP Apr 13, 2023

ManickaP requested a review from antonfirsov April 13, 2023 17:19

ManickaP requested review from CarnaViire, wfurt and rzikm April 13, 2023 17:20

build-analysis bot mentioned this pull request Apr 13, 2023

NuGet restore fails with The repository primary signature validity period has expired dotnet/arcade#13070

Closed

3 tasks

antonfirsov reviewed Apr 13, 2023

View reviewed changes

This was referenced Apr 13, 2023

[wasm] interpreter timeouts when WebSocket closes unexpectedly #84101

Closed

WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434

Closed

ManickaP added 2 commits April 14, 2023 11:10

Feedback: removed code sharing and used reflaction

df7b541

Try to fix missing build dependency

bd6c04d

antonfirsov reviewed Apr 14, 2023

View reviewed changes

rzikm reviewed Apr 17, 2023

View reviewed changes

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs Outdated Show resolved Hide resolved

ManickaP commented Apr 20, 2023

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs Outdated Show resolved Hide resolved

ManickaP force-pushed the mapichov/h3-stress-msquic-hack branch from f85d1af to 8a080e3 Compare April 20, 2023 10:08

Feedback - removed test only function and replaced with shared code +…

fa5820e

… some reflection

ManickaP force-pushed the mapichov/h3-stress-msquic-hack branch from 8a080e3 to fa5820e Compare April 20, 2023 13:06

wfurt reviewed Apr 21, 2023

View reviewed changes

antonfirsov added 3 commits April 25, 2023 21:19

Merge branch 'main' into mapichov/h3-stress-msquic-hack

b649716

fix argument handling in build-local.ps1

ed73b02

do not use MsQuicLibraryVersion

7e4d512

antonfirsov reviewed Apr 26, 2023

View reviewed changes

Use MsQuicLibraryVersion in out internal logging and as a side-effect…

0a9731f

… avoid the property being trimmed

antonfirsov reviewed Apr 27, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

Comment

e316a13

This comment was marked as resolved.

Sign in to view

also copy files to the Linux container

949d82b

antonfirsov approved these changes Apr 27, 2023

View reviewed changes

build-analysis bot mentioned this pull request Apr 28, 2023

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

ManickaP merged commit 2deebf0 into dotnet:main Apr 28, 2023

ManickaP deleted the mapichov/h3-stress-msquic-hack branch April 28, 2023 08:37

runfoapp bot mentioned this pull request Apr 28, 2023

Infra improvements for Helix #68176

Closed

This was referenced May 23, 2023

[HttpStress] massive timeouts around HTTP/3 stress test startup #77126

Closed

Http Stress Status Report #42211

Open

karelz added this to the 8.0.0 milestone May 27, 2023

ManickaP added a commit to ManickaP/runtime that referenced this pull request Jun 22, 2023

Revert dotnet#84793

53fdb50

This was referenced Jun 22, 2023

[HTTP/3] Revert msquic hack in stress tests #87926

Closed

[HttpStress] massive cancellation errors in HTTP/3 #87957

Closed

ghost locked as resolved and limited conversation to collaborators Jun 26, 2023

	// For testing.
	internal HPackDecoder(int maxDynamicTableSize, int maxHeadersLength, DynamicTable dynamicTable)

[HTTP/3] Stress hack for msquic dropping connections #84793

[HTTP/3] Stress hack for msquic dropping connections #84793

Conversation

ManickaP commented Apr 13, 2023

ghost commented Apr 13, 2023

ManickaP commented Apr 13, 2023

azure-pipelines bot commented Apr 13, 2023

antonfirsov Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

antonfirsov Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

antonfirsov Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

antonfirsov Apr 14, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Apr 25, 2023 • 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

Choose a reason for hiding this comment

antonfirsov Apr 25, 2023 • 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

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

ManickaP commented Apr 27, 2023

ManickaP commented Apr 27, 2023

antonfirsov commented Apr 27, 2023

This comment was marked as resolved.

antonfirsov commented Apr 27, 2023

azure-pipelines bot commented Apr 27, 2023

antonfirsov left a comment

Choose a reason for hiding this comment

antonfirsov Apr 13, 2023 •

edited

Loading

antonfirsov Apr 13, 2023 •

edited

Loading

antonfirsov Apr 13, 2023 •

edited

Loading

antonfirsov Apr 14, 2023 •

edited

Loading

antonfirsov Apr 25, 2023 •

edited

Loading

antonfirsov Apr 25, 2023 •

edited

Loading