-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add leeway to revocation test #55571
Conversation
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsWindows 7 has been observed timing out the fetch operation slightly early. For example, 15.9 seconds instead of the expected 16 seconds. Since this is OS behavior that we can't control, the test has been changed to allow a small amount of leeway in the expected timeout. Closes #55554
|
@@ -59,7 +59,9 @@ public static void RevocationCheckingDelayed(PkiOptions pkiOptions) | |||
// OCSP/CRL to root to get intermediate statuses. It should take at least 2x the delay | |||
// plus other non-network time, so we can at least ensure it took as long as | |||
// the delay for each fetch. | |||
Assert.True(watch.Elapsed >= delay * 2, $"watch.Elapsed: {watch.Elapsed}"); | |||
// Some OSes have been observed timing out the AIA fetch slightly early, such as Windows | |||
// 7 timing out at 15.9 seconds instead of 16 seconds. So we allow two seconds of leeway. |
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.
Where does "16 seconds" come from? I see TimeSpan.FromSeconds(15)
above.
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 delay * 2
(delay
is up on line 37). There are two certificates that we are going to check revocation for in this chain, and each one is expected to take 8 seconds since that delay
is a per-certificate timeout. So in general we would expect the whole operation to take at least 16 seconds. But, as this PR is trying to address, Windows 7 appears to be timing out just a bit late.
I see. I will reword the comment since it appears I noted the wrong thing.
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.
Okay - I think I misunderstood myself what was going on here and reworded the comment and tightened the timeout.
Basically, we're doing revocation checking for two certificates. Our fake server introduces an 8 second delay for each response. So we expect the response to complete in >= 16 seconds. But the issue is that it is completing in 15.99 seconds. I suspect this is because the delays are introduced using Thread.Sleep
, which I suspect is imprecise and can complete sooner than 8 seconds (maybe in 7.99 seconds).
Windows 7 has been observed timing out the fetch operation slightly early. For example, 15.9 seconds instead of the expected 16 seconds. Since this is OS behavior that we can't control, the test has been changed to allow a small amount of leeway in the expected timeout.
Closes #55554