- Notifications
You must be signed in to change notification settings - Fork5.2k
Redisign HTTP/2 KeepAlive PING tests#56736
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedAug 2, 2021
Tagging subscribers to this area: @dotnet/ncl Issue DetailsCompletely redesign tests for HTTP/2 KeepAlive PING, so they:
I used a similar pattern as in the dynamic window PR: instead of reading / reacting frames inline, there is a separate Task for processing incoming frames, reacting immediately to PING. Fixes#41929
|
antonfirsov commentedAug 2, 2021
/azp run runtime-libraries-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
alnikola left a comment
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 it possible to get rid of fixed time delays to speed up tests?
| DisablePingResponse(); | ||
| // Simulate inactive period: | ||
| await Task.Delay(6_000); |
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.
Do we really need such long delay here? Is there any way to implement this on synchronization primitives like AutoResetEvent, ManualResetEvent, etc?
antonfirsovAug 3, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Do we really need such long delay here?
By having a big difference between theKeepAlivePingDelayand making sure the tests runs sequentially, we can berelatively safe from CI timing issues. Removing any of these conditions will lead to certain failures IMO.
Note that he minimum value forKeepAlivePingDelay andKeepAlivePingTimeout is 1 sec, so I can't make this faster by using low delay values
Is there any way to implement this on synchronization primitives
I don't see how. The API exposesTimeSpan parameters without any testability-focused abstractions that would allow us to use events.
I understand that the extra ~1 minute added here is very bad from testing perspective, but the only alternative I see is to remove the KeepAlive tests, which means loosing coverage. I wish we used whitebox testing techniques in dotnet/runtime ...
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.
Ok. I got it.
| if (policy == HttpKeepAlivePingPolicy.Always) | ||
| { | ||
| // Simulate inactive period: | ||
| await Task.Delay(5_000); |
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.
Same about a long delay here.
...libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| await Task.Delay(5_000); | ||
| // We may have received one RTT PING in response to HEADERS, but should receive no KeepAlive PING | ||
| Assert.True(_pingCounter <= 1); |
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.
Why don't we count only PINGs relevant to this test and ignore RTT ones? AFAIR, they should have different payload and be distinguishable, no?
antonfirsovAug 3, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 wanted to avoid taking a test-dependency on such an arbitrary implementation detail.
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 actually like the idea of leveraging the different ranges for KeepAlive (>0) and RTT (<0) in tests 😄
We're already counting on details like heartbeat interval and timings, so this doesn't seem to me like something dangerous.
alnikola left a comment
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.
LGTM
ManickaP left a comment
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.
Do we have some outerloop runs after the latest changes?
Otherwise looks good, thanks.
antonfirsov commentedAug 5, 2021
/azp run runtime-libraries-coreclr outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
antonfirsov commentedAug 6, 2021
Outerloop failures are unrelated, mostly instances of#54260 |
Uh oh!
There was an error while loading.Please reload this page.
Completely redesign tests for HTTP/2 KeepAlive PING, so they:
I used a similar pattern as in the dynamic window PR: instead of reading / reacting to frames inline, there is a separate Task for processing incoming frames, responding to PING immediately and pushing other frames to a
Channel<Frame>.Fixes#41929