App-vNext / Polly

Polly is a .NET resilience and transient-fault-handling library that allows developers to express policies such as Retry, Circuit Breaker, Timeout, Bulkhead Isolation, and Fallback in a fluent and thread-safe manner. From version 6.0.1, Polly targets .NET Standard 1.1 and 2.0+.
https://www.thepollyproject.org
BSD 3-Clause "New" or "Revised" License
13.35k stars 1.22k forks source link

Fix issue 2288 #2313

Closed gabidabet closed 5 days ago

gabidabet commented 6 days ago

Pull Request

The issue or feature being addressed

Fixes #2288.

Details on the issue fix or feature implementation

I used a semaphore slim to control the number of parallel tasks that can run.

Confirm the following

codecov[bot] commented 6 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.38%. Comparing base (22d09c0) to head (df6a367). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2313 +/- ## ======================================= Coverage 85.38% 85.38% ======================================= Files 313 313 Lines 7457 7457 Branches 1126 1126 ======================================= Hits 6367 6367 Misses 745 745 Partials 345 345 ``` | [Flag](https://app.codecov.io/gh/App-vNext/Polly/pull/2313/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=App-vNext) | Coverage Δ | | |---|---|---| | [linux](https://app.codecov.io/gh/App-vNext/Polly/pull/2313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=App-vNext) | `85.38% <ø> (+0.02%)` | :arrow_up: | | [macos](https://app.codecov.io/gh/App-vNext/Polly/pull/2313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=App-vNext) | `85.38% <ø> (ø)` | | | [windows](https://app.codecov.io/gh/App-vNext/Polly/pull/2313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=App-vNext) | `85.38% <ø> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=App-vNext#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

martincostello commented 6 days ago

This seems to resolve the original issue, but I've asked the xunit maintainer who proposed the original workaround to take a look to give their opinion. If we don't hear from them within a week, I'll merge this as-is.

bradwilson commented 5 days ago

Yeah, this seems like a reasonable way to ensure that there is no dependency on the current thread.

The only suggestion I'll make is to change semaphore.Wait(); into one that waits a limited amount of time (long enough to not give false positives, which can be facilitated by putting the test into a test collection that is marked as non-parallelized), and then failing if the wait did not succeed. This will allow the test to wait and fail if somehow the production code stops behaving appropriately, rather than waiting forever (and requiring a "long running tests" detection pass to determine which test is problematic).

martincostello commented 5 days ago

Good suggestion - thanks Brad!

I reckon about 20 seconds should be long enough for our purposes to guard against a "forever wait".

gabidabet commented 5 days ago

Ok I will add a timeout of 20s.