Sleep in MultipleThreadsForceRefresh

[amcasey]

...to increase the likelihood of two threads racing to enter the critical section.

For #55227

[amcasey]

FYI @BrennanConroy, the person most likely to be unhappy about the sleep. 😆

[BrennanConroy]

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.

[amcasey]

I'm not taking the blame for that e2e failure. 😝

[amcasey]

I plan to azp run at least twice before merging.

[amcasey]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[halter73]

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?

[amcasey]

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

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.

[halter73]

Where would the dedicated test hook have to go?

[amcasey]

Where would the dedicated test hook have to go?

Inside this lock.

[halter73]

Given that this is testing forceRefresh: true case, I'm not sure how important it is to test that in extremely rare cases, if a parallel thread is executing at exactly the same time (with the same DateTimeOffset.UtcNow.Ticks), you might get a cached value anyway. As long as the results are what we expect, it'd probably be fine if the parallel threads always make their own call to ICacheableKeyRingProvider.GetCacheableKeyRing(DateTimeOffset).

If this really is important, I think we should add a test hook, but otherwise I'd just delete the Times.AtMost(taskCount - 1) verification part of the test.

[amcasey]

Given that this is testing forceRefresh: true case, I'm not sure how important it is to test that in extremely rare cases, if a parallel thread is executing at exactly the same time (with the same DateTimeOffset.UtcNow.Ticks), you might get a cached value anyway. As long as the results are what we expect, it'd probably be fine if the parallel threads always make their own call to ICacheableKeyRingProvider.GetCacheableKeyRing(DateTimeOffset).

I agree that the test adds little value and would be happy to delete it.

If this really is important, I think we should add a test hook, but otherwise I'd just delete the Times.AtMost(taskCount - 1) verification part of the test.

The rest of the functionality is covered by other tests.

Edit: #55290