Redisign HTTP/2 KeepAlive PING tests #56736
Conversation
|
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
|
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| DisablePingResponse(); | ||
|
|
||
| // Simulate inactive period: | ||
| await Task.Delay(6_000); |
There was a problem hiding this comment.
Do we really need such long delay here? Is there any way to implement this on synchronization primitives like AutoResetEvent, ManualResetEvent, etc?
There was a problem hiding this comment.
Do we really need such long delay here?
By having a big difference between the KeepAlivePingDelay and making sure the tests runs sequentially, we can be relatively safe from CI timing issues. Removing any of these conditions will lead to certain failures IMO.
Note that he minimum value for KeepAlivePingDelay and KeepAlivePingTimeout 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 exposes TimeSpan 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 ...
| if (policy == HttpKeepAlivePingPolicy.Always) | ||
| { | ||
| // Simulate inactive period: | ||
| await Task.Delay(5_000); |
There was a problem hiding this comment.
Same about a long delay here.
...libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs
Outdated
Show resolved
Hide resolved
| 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.
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?
There was a problem hiding this comment.
I wanted to avoid taking a test-dependency on such an arbitrary implementation detail.
There was a problem hiding this comment.
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.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Outerloop failures are unrelated, mostly instances of #54260 |
ghost
locked as resolved and limited conversation to collaborators
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