-
Notifications
You must be signed in to change notification settings - Fork 1k
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
harden CircuitBreakerSpecs.cs
#7418
harden CircuitBreakerSpecs.cs
#7418
Conversation
harden `CircuitBreakerSpecs.Must_increment_failure_count_on_callTimeout_before_call_finishes`
@@ -85,7 +85,8 @@ public async Task Must_increment_failure_count_on_callTimeout_before_call_finish | |||
var breaker = ShortCallTimeoutCb(); | |||
#pragma warning disable CS4014 // Because this call is not awaited, execution of the current method continues before the call is completed | |||
// meant to run as detached task | |||
Task.Run(() => breaker.Instance.WithSyncCircuitBreaker(() => Thread.Sleep(Dilated(TimeSpan.FromSeconds(1))))); | |||
var t = Task.Run(() => breaker.Instance.WithSyncCircuitBreaker(() => Thread.Sleep(Dilated(TimeSpan.FromSeconds(1))))); | |||
await AwaitConditionAsync(() => t.Status >= TaskStatus.Running); // need to kick off the task before we can check the latch |
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.
This spec is pretty racy due to non-determinism of the TaskScheduler
- now we force our assertions to wait until the Task.Run
actually gets scheduled for execution, because otherwise the breaker
never gets a chance to be scheduled and this will throw the timing off.
_ = breaker.Instance.WithCircuitBreaker(() => Task.Run(SayHi)); | ||
Enumerable.Range(1, 4).ForEach(_ => breaker.Instance.WithCircuitBreaker(() => Task.Run(ThrowException))); | ||
await WaitForTaskToBeScheduled(breaker.Instance.WithCircuitBreaker(() => Task.Run(SayHi))); | ||
await WaitForTaskToBeScheduled(Enumerable.Range(1, 4).Select(_ => breaker.Instance.WithCircuitBreaker(() => Task.Run(ThrowException))).ToList()); |
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.
Issues we've been running into this spec are due to scheduler lag again - when the CPU count is low these tasks may not get scheduled to run immediately.
These circuit breaker specs are kind of sketchy in the first place IMHO, as many of them rely on imprecise system clock behavior and the non-deterministic scheduling of the |
This has been impacted by #7419 a couple of times; we can debug that separately |
Changes
harden
CircuitBreakerSpecs.Must_increment_failure_count_on_callTimeout_before_call_finishes
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):