-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat!: Add support for provider shutdown and status. #158
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Extensions.Logging; | ||
using OpenFeature.Model; | ||
|
||
|
@@ -15,14 +16,12 @@ namespace OpenFeature | |
public sealed class Api | ||
{ | ||
private EvaluationContext _evaluationContext = EvaluationContext.Empty; | ||
private FeatureProvider _defaultProvider = new NoOpFeatureProvider(); | ||
private readonly ConcurrentDictionary<string, FeatureProvider> _featureProviders = | ||
new ConcurrentDictionary<string, FeatureProvider>(); | ||
private readonly ProviderRepository _repository = new ProviderRepository(); | ||
private readonly ConcurrentStack<Hook> _hooks = new ConcurrentStack<Hook>(); | ||
|
||
/// The reader/writer locks are not disposed because the singleton instance should never be disposed. | ||
private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim(); | ||
private readonly ReaderWriterLockSlim _featureProviderLock = new ReaderWriterLockSlim(); | ||
|
||
|
||
/// <summary> | ||
/// Singleton instance of Api | ||
|
@@ -36,31 +35,26 @@ static Api() { } | |
private Api() { } | ||
|
||
/// <summary> | ||
/// Sets the feature provider | ||
/// Sets the feature provider. In order to wait for the provider to be set, and initialization to complete, | ||
/// await the returned task. | ||
/// </summary> | ||
/// <remarks>The provider cannot be set to null. Attempting to set the provider to null has no effect.</remarks> | ||
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param> | ||
public void SetProvider(FeatureProvider featureProvider) | ||
public async Task SetProvider(FeatureProvider featureProvider) | ||
{ | ||
this._featureProviderLock.EnterWriteLock(); | ||
try | ||
{ | ||
this._defaultProvider = featureProvider ?? this._defaultProvider; | ||
} | ||
finally | ||
{ | ||
this._featureProviderLock.ExitWriteLock(); | ||
} | ||
await this._repository.SetProvider(featureProvider, this.GetContext()).ConfigureAwait(false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored the collection management out of the Api and moved it into the ProviderRepository. This is similar to the Java SDK. |
||
} | ||
|
||
|
||
/// <summary> | ||
/// Sets the feature provider to given clientName | ||
/// Sets the feature provider to given clientName. In order to wait for the provider to be set, and | ||
/// initialization to complete, await the returned task. | ||
/// </summary> | ||
/// <param name="clientName">Name of client</param> | ||
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param> | ||
public void SetProvider(string clientName, FeatureProvider featureProvider) | ||
public async Task SetProvider(string clientName, FeatureProvider featureProvider) | ||
{ | ||
this._featureProviders.AddOrUpdate(clientName, featureProvider, | ||
(key, current) => featureProvider); | ||
await this._repository.SetProvider(clientName, featureProvider, this.GetContext()).ConfigureAwait(false); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -76,15 +70,7 @@ public void SetProvider(string clientName, FeatureProvider featureProvider) | |
/// <returns><see cref="FeatureProvider"/></returns> | ||
public FeatureProvider GetProvider() | ||
{ | ||
this._featureProviderLock.EnterReadLock(); | ||
try | ||
{ | ||
return this._defaultProvider; | ||
} | ||
finally | ||
{ | ||
this._featureProviderLock.ExitReadLock(); | ||
} | ||
return this._repository.GetProvider(); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -95,17 +81,9 @@ public FeatureProvider GetProvider() | |
/// have a corresponding provider the default provider will be returned</returns> | ||
public FeatureProvider GetProvider(string clientName) | ||
{ | ||
if (string.IsNullOrEmpty(clientName)) | ||
{ | ||
return this.GetProvider(); | ||
} | ||
|
||
return this._featureProviders.TryGetValue(clientName, out var featureProvider) | ||
? featureProvider | ||
: this.GetProvider(); | ||
return this._repository.GetProvider(clientName); | ||
} | ||
|
||
|
||
/// <summary> | ||
/// Gets providers metadata | ||
/// <para> | ||
|
@@ -210,5 +188,19 @@ public EvaluationContext GetContext() | |
this._evaluationContextLock.ExitReadLock(); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// <para> | ||
/// Shut down and reset the current status of OpenFeature API. | ||
/// </para> | ||
/// <para> | ||
/// This call cleans up all active providers and attempts to shut down internal event handling mechanisms. | ||
/// Once shut down is complete, API is reset and ready to use again. | ||
/// </para> | ||
/// </summary> | ||
public async Task Shutdown() | ||
{ | ||
await this._repository.Shutdown().ConfigureAwait(false); | ||
} | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
using System.ComponentModel; | ||
|
||
namespace OpenFeature.Constant | ||
{ | ||
/// <summary> | ||
/// The state of the provider. | ||
/// </summary> | ||
/// <seealso href="https://github.com/open-feature/spec/blob/main/specification/sections/02-providers.md#requirement-242" /> | ||
public enum ProviderStatus | ||
{ | ||
/// <summary> | ||
/// The provider has not been initialized and cannot yet evaluate flags. | ||
/// </summary> | ||
[Description("NOT_READY")] NotReady, | ||
|
||
/// <summary> | ||
/// The provider is ready to resolve flags. | ||
/// </summary> | ||
[Description("READY")] Ready, | ||
|
||
/// <summary> | ||
/// The provider's cached state is no longer valid and may not be up-to-date with the source of truth. | ||
/// </summary> | ||
[Description("STALE")] Stale, | ||
|
||
/// <summary> | ||
/// The provider is in an error state and unable to evaluate flags. | ||
/// </summary> | ||
[Description("ERROR")] Error | ||
} | ||
} |
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
Oops, something went wrong.
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.
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.
So, returning a task here is a breaking API change. This seems like the most appropriate action. Await the method if you care that it finishes initialization, don't await if you do not.
Other options would be adding another method, but this method would just be async without a way to know it finished, versus being sync.
This all applies to named providers as well.
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.
Yea I've been thinking about this myself its quite common to offer bother sync and async libraries especially when working on a old legacy project in full framework. Lets just leave it for now and see if anyone raise concern before going ahead an introduce a sync based methods.
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.
"events" are the mechanism by which you can call this synchronously and "know" the outcome (your READY/ERROR event handlers will run). I know they don't exist on this SDK yet, but they will eventually.
This is what's been done in Java and JS, and what the spec recommends: https://openfeature.dev/specification/sections/flag-evaluation/#requirement-118
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.
So, the spec makes a lot of sense for Java. Complete-able futures are not super convenient, and you would probably need something like kotlin on top to make a useful API out of it.
Dotnet though has reasonably proper async, and the general means would be a method you either await, or don't await, or synchronously wait.
The language in the spec doesn't really work, and I am not sure what would.
Usually if you have a sync and async method you would have.
doTheThing
anddoTheThingAsync
. But,doTheThing
would be synchronous, not async without completion.The spec recommends
setProviderAndWait
, which implies sync, but I don't think we actually want to call.Wait
on behalf of the user. They can make the choice to asynchronously await, not await or wait, or to synchronously wait, and we don't have to worry about quirks in their thread execution model that could lead to things like a deadlock.Either approach will result in an API that is not intuitive for the ecosystem. It can be done, it is just unfortunate.
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.
(To elaborate
AndWait
orWait
in general have been used to provide sync APIs where the base API is async, so it would be a confusing name)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.
I think, if the API is fully async, then we don't need to worry about the Async suffix. It really is a case of libraries that implemented things synchronously, and then needed async methods as well.
The interface for this API is almost all Async, and it happened that there was this sync method, so making it async would provide a consistent async interface.
It may be worth checking the rest of the API to see if other items should move to async with this.
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.
(I guess the client is really where things are async, and the "Api" doesn't have that many methods that are async, or would conceivably be async.)
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.
I'm fine with keeping it all async and not providing a sync api. I think since we aren't 1.x.x yet its the best time to make a breaking change to SetProvider, i think we just need to call it out in the release notes.
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 SDK is already version
1.3.1
but we can release a version2.0
if it makes sense. I think it does make sense in this case since it simplifies waiting for a provider to initialize and the migration process should be straightforward.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.
I'm fine with the breaking change, especially if we agree we'll do the same breaking change in JS, which has the same async/await semantics.