-
Notifications
You must be signed in to change notification settings - Fork 9.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
Sleep in MultipleThreadsForceRefresh #55231
Conversation
...to increase the likelihood of two threads racing to enter the critical section. Fixes dotnet#55227
FYI @BrennanConroy, the person most likely to be unhappy about the sleep. 😆 |
...tection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs
Show resolved
Hide resolved
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.
Not the biggest fan, but the only real way to make this test deterministic is to modify product code to make it more testable. We can see if this makes the test reliable enough (4 9's?) and re-think the code if it fails more often.
I'm not taking the blame for that e2e failure. 😝 |
I plan to azp run at least twice before merging. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Any test that relies on a Thread.Sleep is immediately suspect. It's usually better to use something like a ManualResetSlim to unblock the thread only after you've confirmed the race you're trying to induce has occurred. That way you can make the max wait longer than 200 ms if need be but finish the test a lot faster than 200 ms in the majority of cases. You never know when other work might get preempted an outwait the Thread.Sleep no matter how long you make it. I understand the urgency though. I've seen this test start failing pretty consistently on unrelated PRs like #55209. Can we combine this PR with #55228 which retries it and quarantine the test? |
Agreed, but neither @BrennanConroy nor I could figure out a way to do that without adding a dedicated test hook. I don't think retrying makes sense once the test is quarantined, but I agree that quarantining should be part of this PR. |
Where would the dedicated test hook have to go? |
Inside this lock. |
Given that this is testing If this really is important, I think we should add a test hook, but otherwise I'd just delete the |
I agree that the test adds little value and would be happy to delete it.
The rest of the functionality is covered by other tests. Edit: #55290 |
...to increase the likelihood of two threads racing to enter the critical section.
For #55227