-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HTTP/3] Stress hack for msquic dropping connections #84793
Changes from 3 commits
df45d59
df7b541
bd6c04d
fa5820e
b649716
ed73b02
7e4d512
1f8f56e
d2c113c
7613f06
0a9731f
e316a13
949d82b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -31,6 +31,7 @@ internal sealed unsafe partial class MsQuicApi | |||||
// Remove once fixed: https://github.com/mono/linker/issues/1660 | ||||||
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicConstructors, typeof(MsQuicSafeHandle))] | ||||||
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicConstructors, typeof(MsQuicContextSafeHandle))] | ||||||
[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(MsQuicApi))] | ||||||
ManickaP marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
private MsQuicApi(QUIC_API_TABLE* apiTable) | ||||||
{ | ||||||
ApiTable = apiTable; | ||||||
|
@@ -227,4 +228,19 @@ private static bool IsTls13Disabled(bool isServer) | |||||
#endif | ||||||
return false; | ||||||
} | ||||||
|
||||||
// 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() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Combo of shared code + reflection is there. @antonfirsov could you look into copying the appropriate files in the docker container please? |
||||||
{ | ||||||
if (!IsQuicSupported) | ||||||
{ | ||||||
return (true, MsQuicLibraryVersion); | ||||||
} | ||||||
|
||||||
QUIC_SETTINGS settings = default(QUIC_SETTINGS); | ||||||
settings.IsSet.MaxWorkerQueueDelayUs = 1; | ||||||
settings.MaxWorkerQueueDelayUs = 2_500_000u; // 2.5s, 10x the default | ||||||
return (StatusSucceeded(MsQuicApi.Api.ApiTable->SetParam(null, QUIC_PARAM_GLOBAL_SETTINGS, (uint)sizeof(QUIC_SETTINGS), (byte*)&settings)), MsQuicLibraryVersion); | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8.0 will not support Centos 7 AFAIK (hurray!) we should update this at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stream9??? Or should we swap for different distro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are running stress tests on Centos and not ubuntu, unlike enterprise tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm switching to
centos-stream8
in #85342, which seems to work.