fix: More robust shutdown/cleanup/reset #188
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
austindrenski
force-pushed
the
more-robust-shutdown-cleanup-and-reset
branch
from
4c5d79f
to
2f17a64
Compare
Codecov ReportAttention:
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
austindrenski
added a commit
to austindrenski/open-feature-dotnet-sdk
that referenced
this pull request
Jan 17, 2024
10 tasks
austindrenski
added a commit
to austindrenski/open-feature-dotnet-sdk
that referenced
this pull request
Jan 17, 2024
austindrenski
added a commit
to austindrenski/open-feature-dotnet-sdk
that referenced
this pull request
Jan 17, 2024
austindrenski
force-pushed
the
more-robust-shutdown-cleanup-and-reset
branch
from
2f17a64
to
66bf351
Compare
askpt
reviewed
Jan 18, 2024
austindrenski
force-pushed
the
more-robust-shutdown-cleanup-and-reset
branch
from
66bf351
to
2c197d8
Compare
askpt
approved these changes
Jan 18, 2024
toddbaert
approved these changes
Jan 19, 2024
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. |
Previously, we shutdown the consumer thread causing any reuse of the Api to block on the second event emitted to the event executor. Fixes: open-feature#186 Signed-off-by: Austin Drenski <[email protected]>
austindrenski
force-pushed
the
more-robust-shutdown-cleanup-and-reset
branch
from
14aacac
to
f8ada7d
Compare
austindrenski
added a commit
to austindrenski/open-feature-dotnet-sdk
that referenced
this pull request
Jan 19, 2024
beeme1mr
pushed a commit
that referenced
this pull request
Apr 9, 2024
This PR updates `OpenFeature` to use the latest MELA source generators to produce compile-time logging delegates. The most obvious critique of this PR is likely to be that, much like #188, this PR changes the dependency graph by bumping our MELA dependency from `[2.0.0,) => [8.0.0,)`. To that^ critique, I continue to contend that depending on an ancient version of MELA is unlikely to aid many real-world consumers of this SDK, since new versions of MELA provide robust TFM coverage, as well as my personal disbelief that anyone looking to consume this library in 2024 is deploying an app that won't already have transitively referenced MELA `8.0.0` somewhere in its package graph. _(If you are a user or potential user of this SDK and have a real-world use case of a legacy app that __does not and cannot__ reference MELA `>= 8.0.0`, please ping back here! Would love to hear from you and adjust my disbelief accordingly!)_ _(Note: if this PR lands before #188, then I'll update #188 to remove its added dependency on `Microsoft.Bcl.AsyncInterfaces`, since it flows transitively from MELA `8.0.0`.)_ Upon request, I am happy to provide a soapbox diatribe on why I think we should care about source generators, hook perf, and incidental logging allocations, especially as an SDK that wants to be trusted and baked into all kinds of consumer apps, but eliding that for now in favor of some docs refs: - https://learn.microsoft.com/dotnet/core/extensions/high-performance-logging - https://learn.microsoft.com/dotnet/core/extensions/logger-message-generator Signed-off-by: Austin Drenski <[email protected]> Signed-off-by: André Silva <[email protected]> Co-authored-by: André Silva <[email protected]>
askpt
added a commit
that referenced
this pull request
Apr 11, 2024
This PR updates `OpenFeature` to use the latest MELA source generators to produce compile-time logging delegates. The most obvious critique of this PR is likely to be that, much like #188, this PR changes the dependency graph by bumping our MELA dependency from `[2.0.0,) => [8.0.0,)`. To that^ critique, I continue to contend that depending on an ancient version of MELA is unlikely to aid many real-world consumers of this SDK, since new versions of MELA provide robust TFM coverage, as well as my personal disbelief that anyone looking to consume this library in 2024 is deploying an app that won't already have transitively referenced MELA `8.0.0` somewhere in its package graph. _(If you are a user or potential user of this SDK and have a real-world use case of a legacy app that __does not and cannot__ reference MELA `>= 8.0.0`, please ping back here! Would love to hear from you and adjust my disbelief accordingly!)_ _(Note: if this PR lands before #188, then I'll update #188 to remove its added dependency on `Microsoft.Bcl.AsyncInterfaces`, since it flows transitively from MELA `8.0.0`.)_ Upon request, I am happy to provide a soapbox diatribe on why I think we should care about source generators, hook perf, and incidental logging allocations, especially as an SDK that wants to be trusted and baked into all kinds of consumer apps, but eliding that for now in favor of some docs refs: - https://learn.microsoft.com/dotnet/core/extensions/high-performance-logging - https://learn.microsoft.com/dotnet/core/extensions/logger-message-generator Signed-off-by: Austin Drenski <[email protected]> Signed-off-by: André Silva <[email protected]> Co-authored-by: André Silva <[email protected]> Signed-off-by: André Silva <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IHostand then dispose of it at the conclusion of each test case.But since the
Api.Instanceis 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 thatMicrosoft.Bcl.AsyncInterfacesis a transitive dependency ofMicrosoft.Extensions.DependencyInjection.Abstractionswhich itself is a transitive dependency ofMicrosoft.Extensions.Logging.Abstractions >= 8.0.0, and only applies to TFMs that do not shipIAsyncDisposableand friends in-the-box, i.e.,net462andnetstandard2.0.An even cleaner solution would be to just bump our minimum required version of
Microsoft.Extensions.Logging.Abstractionsto8.0.0at which pointMicrosoft.Bcl.AsyncInterfacescomes 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.)