Skip to content
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 2 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ dotnet add package OpenFeature
public async Task Example()
{
// Register your feature flag provider
Api.Instance.SetProvider(new InMemoryProvider());
await Api.Instance.SetProvider(new InMemoryProvider());

// Create a new client
FeatureClient client = Api.Instance.GetClient();
Expand All @@ -97,7 +97,7 @@ public async Task Example()
| ✅ | [Logging](#logging) | Integrate with popular logging packages. |
| ✅ | [Named clients](#named-clients) | Utilize multiple providers in a single application. |
| ❌ | [Eventing](#eventing) | React to state changes in the provider or flag management system. |
| | [Shutdown](#shutdown) | Gracefully clean up a provider during application shutdown. |
| | [Shutdown](#shutdown) | Gracefully clean up a provider during application shutdown. |
| ✅ | [Extending](#extending) | Extend OpenFeature with custom providers and hooks. |

<sub>Implemented: ✅ | In-progress: ⚠️ | Not implemented yet: ❌</sub>
Expand All @@ -112,7 +112,7 @@ If the provider you're looking for hasn't been created yet, see the [develop a p
Once you've added a provider as a dependency, it can be registered with OpenFeature like this:

```csharp
Api.Instance.SetProvider(new MyProvider());
await Api.Instance.SetProvider(new MyProvider());
```

In some situations, it may be beneficial to register multiple providers in the same application.
Expand Down Expand Up @@ -179,9 +179,9 @@ If a name has no associated provider, the global provider is used.

```csharp
// registering the default provider
Api.Instance.SetProvider(new LocalProvider());
await Api.Instance.SetProvider(new LocalProvider());
// registering a named provider
Api.Instance.SetProvider("clientForCache", new CachedProvider());
await Api.Instance.SetProvider("clientForCache", new CachedProvider());

// a client backed by default provider
FeatureClient clientDefault = Api.Instance.GetClient();
Expand All @@ -196,7 +196,12 @@ Events are currently not supported by the .NET SDK. Progress on this feature can

### Shutdown

A shutdown handler is not yet available in the .NET SDK. Progress on this feature can be tracked [here](https://github.com/open-feature/dotnet-sdk/issues/126).
The OpenFeature API provides a close function to perform a cleanup of all registered providers. This should only be called when your application is in the process of shutting down.

```csharp
// Shut down all providers
await Api.Instance.Shutdown();
```

## Extending

Expand Down
66 changes: 29 additions & 37 deletions src/OpenFeature/Api.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand All @@ -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)
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

"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.

Other options would be adding another method

This is what's been done in Java and JS, and what the spec recommends: https://openfeature.dev/specification/sections/flag-evaluation/#requirement-118

Copy link
Member Author

@kinyoklion kinyoklion Nov 1, 2023

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 and doTheThingAsync. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To elaborate AndWait or Wait in general have been used to provide sync APIs where the base API is async, so it would be a confusing name)

Copy link
Member Author

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.

Copy link
Member Author

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.)

Copy link
Member

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.

Copy link
Member

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 version 2.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.

Copy link
Member

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.

{
this._featureProviderLock.EnterWriteLock();
try
{
this._defaultProvider = featureProvider ?? this._defaultProvider;
}
finally
{
this._featureProviderLock.ExitWriteLock();
}
await this._repository.SetProvider(featureProvider, this.GetContext()).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

The 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>
Expand All @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -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);
}
}
}
31 changes: 31 additions & 0 deletions src/OpenFeature/Constant/ProviderStatus.cs
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
}
}
49 changes: 49 additions & 0 deletions src/OpenFeature/FeatureProvider.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System.Collections.Immutable;
using System.Threading.Tasks;
using OpenFeature.Constant;
using OpenFeature.Model;

namespace OpenFeature
Expand Down Expand Up @@ -79,5 +80,53 @@
/// <returns><see cref="ResolutionDetails{T}"/></returns>
public abstract Task<ResolutionDetails<Value>> ResolveStructureValue(string flagKey, Value defaultValue,
EvaluationContext context = null);

/// <summary>
/// Get the status of the provider.
/// </summary>
/// <returns>The current <see cref="ProviderStatus"/></returns>
/// <remarks>
/// If a provider does not override this method, then its status will be assumed to be
/// <see cref="ProviderStatus.Ready"/>. If a provider implements this method, and supports initialization,
/// then it should start in the <see cref="ProviderStatus.NotReady"/>status . If the status is
/// <see cref="ProviderStatus.NotReady"/>, then the Api will call the <see cref="Initialize" /> when the
/// provider is set.
/// </remarks>
public virtual ProviderStatus GetStatus() => ProviderStatus.Ready;

/// <summary>
/// <para>
/// This method is called before a provider is used to evaluate flags. Providers can overwrite this method,
/// if they have special initialization needed prior being called for flag evaluation.
/// </para>
/// </summary>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <returns>A task that completes when the initialization process is complete.</returns>
/// <remarks>
/// <para>
/// A provider which supports initialization should override this method as well as
/// <see cref="FeatureProvider.GetStatus"/>.
/// </para>
/// <para>
/// The provider should return <see cref="ProviderStatus.Ready"/> or <see cref="ProviderStatus.Error"/> from
/// the <see cref="GetStatus"/> method after initialization is complete.
/// </para>
/// </remarks>
public virtual Task Initialize(EvaluationContext context)
{

Check warning on line 116 in src/OpenFeature/FeatureProvider.cs

View check run for this annotation

Codecov / codecov/patch

src/OpenFeature/FeatureProvider.cs#L116

Added line #L116 was not covered by tests
// Intentionally left blank.
return Task.CompletedTask;
}

Check warning on line 119 in src/OpenFeature/FeatureProvider.cs

View check run for this annotation

Codecov / codecov/patch

src/OpenFeature/FeatureProvider.cs#L118-L119

Added lines #L118 - L119 were not covered by tests

/// <summary>
/// This method is called when a new provider is about to be used to evaluate flags, or the SDK is shut down.
/// Providers can overwrite this method, if they have special shutdown actions needed.
/// </summary>
/// <returns>A task that completes when the shutdown process is complete.</returns>
public virtual Task Shutdown()
{
// Intentionally left blank.
return Task.CompletedTask;
}
}
}
Loading