search

open-feature / dotnet-sdk

.NET implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
69 stars 19 forks source link

fix: More robust shutdown/cleanup/reset #188

Closed austindrenski closed 9 months ago

austindrenski commented 9 months ago

Previously, we shutdown the consumer thread causing any reuse of the Api to block on the second event emitted to the event executor.

Fixes: #186

Stumbled into this one while working on #181 in which I've added unit tests that each setup their own independent IHost and then dispose of it at the conclusion of each test case.

But since the Api.Instance is inherently static/shared, subsequent test cases were observed blocking until CI timeout occurred.

This PR could reasonably be criticized for going a step too far by introducing a new dependency on Microsoft.Bcl.AsyncInterfaces, but I decided to propose it anyways on the basis that Microsoft.Bcl.AsyncInterfaces is a transitive dependency of Microsoft.Extensions.DependencyInjection.Abstractions which itself is a transitive dependency of Microsoft.Extensions.Logging.Abstractions >= 8.0.0, and only applies to TFMs that do not ship IAsyncDisposable and friends in-the-box, i.e., net462 and netstandard2.0.

An even cleaner solution would be to just bump our minimum required version of Microsoft.Extensions.Logging.Abstractions to 8.0.0 at which point Microsoft.Bcl.AsyncInterfaces comes transitively for free. I would prefer to do this, but figured that might be too controversial right before a release. (That said, if you like this idea too, please chime in because I have a strong belief that exceedingly few consumers in the wild actually need us to keep our MELA dependency pinned back in 2017. The newer versions of MELA are available for all of the relevant TFMs, which ought to mean there's no dependency hell argument at play on this one.)

codecov[bot] commented 9 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1a14f6c) 0.00% compared to head (f8ada7d) 93.86%.

Files Patch % Lines
src/OpenFeature/EventExecutor.cs 90.90% 2 Missing :warning:
src/OpenFeature/Api.cs 94.11% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #188 +/- ## ========================================= + Coverage 0 93.86% +93.86% ========================================= Files 0 23 +23 Lines 0 946 +946 Branches 0 105 +105 ========================================= + Hits 0 888 +888 - Misses 0 34 +34 - Partials 0 24 +24 ```

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

toddbaert commented 9 months ago

An even cleaner solution would be to just bump our minimum required version of Microsoft.Extensions.Logging.Abstractions to 8.0.0 at which point Microsoft.Bcl.AsyncInterfaces comes transitively for free. I would prefer to do this, but figured that might be too controversial right before a release.

Let's do this after this upcoming release, maybe?

austindrenski commented 9 months ago

An even cleaner solution would be to just bump our minimum required version of Microsoft.Extensions.Logging.Abstractions to 8.0.0 at which point Microsoft.Bcl.AsyncInterfaces comes transitively for free. I would prefer to do this, but figured that might be too controversial right before a release.

Let's do this after this upcoming release, maybe?

Yeah, that's sounds fair. Plus, it'll dovetail nicely with #189 which also shouldn't land until after the upcoming release is finalized.