From a790f78c32b8500ce27d04d906c98d1de2afd2b4 Mon Sep 17 00:00:00 2001 From: Austin Drenski Date: Fri, 19 Jan 2024 16:24:33 -0500 Subject: [PATCH] fix: More robust shutdown/cleanup/reset (#188) Signed-off-by: Austin Drenski --- build/Common.props | 3 +- src/OpenFeature/Api.cs | 34 ++++--- src/OpenFeature/EventExecutor.cs | 102 +++++++-------------- src/OpenFeature/OpenFeature.csproj | 2 + src/OpenFeature/OpenFeatureClient.cs | 4 +- src/OpenFeature/ProviderRepository.cs | 10 +- test/OpenFeature.Tests/OpenFeatureTests.cs | 8 -- 7 files changed, 69 insertions(+), 94 deletions(-) diff --git a/build/Common.props b/build/Common.props index 65bf984d..c33ed3ec 100644 --- a/build/Common.props +++ b/build/Common.props @@ -1,6 +1,6 @@ - 7.3 + latest true true @@ -19,6 +19,7 @@ Please sort alphabetically. Refer to https://docs.microsoft.com/nuget/concepts/package-versioning for semver syntax. --> + [8.0.0,) [2.0,) [1.0.0,2.0) diff --git a/src/OpenFeature/Api.cs b/src/OpenFeature/Api.cs index 440242da..e2690a7a 100644 --- a/src/OpenFeature/Api.cs +++ b/src/OpenFeature/Api.cs @@ -17,15 +17,13 @@ namespace OpenFeature public sealed class Api : IEventBus { private EvaluationContext _evaluationContext = EvaluationContext.Empty; - private readonly ProviderRepository _repository = new ProviderRepository(); + private EventExecutor _eventExecutor = new EventExecutor(); + private ProviderRepository _repository = new ProviderRepository(); private readonly ConcurrentStack _hooks = new ConcurrentStack(); /// The reader/writer locks are not disposed because the singleton instance should never be disposed. private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim(); - internal readonly EventExecutor EventExecutor = new EventExecutor(); - - /// /// Singleton instance of Api /// @@ -45,7 +43,7 @@ private Api() { } /// Implementation of public async Task SetProvider(FeatureProvider featureProvider) { - this.EventExecutor.RegisterDefaultFeatureProvider(featureProvider); + this._eventExecutor.RegisterDefaultFeatureProvider(featureProvider); await this._repository.SetProvider(featureProvider, this.GetContext()).ConfigureAwait(false); } @@ -58,7 +56,7 @@ public async Task SetProvider(FeatureProvider featureProvider) /// Implementation of public async Task SetProvider(string clientName, FeatureProvider featureProvider) { - this.EventExecutor.RegisterClientFeatureProvider(clientName, featureProvider); + this._eventExecutor.RegisterClientFeatureProvider(clientName, featureProvider); await this._repository.SetProvider(clientName, featureProvider, this.GetContext()).ConfigureAwait(false); } @@ -224,20 +222,28 @@ public EvaluationContext GetContext() /// public async Task Shutdown() { - await this._repository.Shutdown().ConfigureAwait(false); - await this.EventExecutor.Shutdown().ConfigureAwait(false); + await using (this._eventExecutor.ConfigureAwait(false)) + await using (this._repository.ConfigureAwait(false)) + { + this._evaluationContext = EvaluationContext.Empty; + this._hooks.Clear(); + + // TODO: make these lazy to avoid extra allocations on the common cleanup path? + this._eventExecutor = new EventExecutor(); + this._repository = new ProviderRepository(); + } } /// public void AddHandler(ProviderEventTypes type, EventHandlerDelegate handler) { - this.EventExecutor.AddApiLevelHandler(type, handler); + this._eventExecutor.AddApiLevelHandler(type, handler); } /// public void RemoveHandler(ProviderEventTypes type, EventHandlerDelegate handler) { - this.EventExecutor.RemoveApiLevelHandler(type, handler); + this._eventExecutor.RemoveApiLevelHandler(type, handler); } /// @@ -246,7 +252,13 @@ public void RemoveHandler(ProviderEventTypes type, EventHandlerDelegate handler) /// The logger to be used public void SetLogger(ILogger logger) { - this.EventExecutor.Logger = logger; + this._eventExecutor.Logger = logger; } + + internal void AddClientHandler(string client, ProviderEventTypes eventType, EventHandlerDelegate handler) + => this._eventExecutor.AddClientHandler(client, eventType, handler); + + internal void RemoveClientHandler(string client, ProviderEventTypes eventType, EventHandlerDelegate handler) + => this._eventExecutor.RemoveClientHandler(client, eventType, handler); } } diff --git a/src/OpenFeature/EventExecutor.cs b/src/OpenFeature/EventExecutor.cs index 8a6df9a4..7bdfeb6e 100644 --- a/src/OpenFeature/EventExecutor.cs +++ b/src/OpenFeature/EventExecutor.cs @@ -10,19 +10,13 @@ namespace OpenFeature { - - internal delegate Task ShutdownDelegate(); - - internal class EventExecutor + internal class EventExecutor : IAsyncDisposable { private readonly object _lockObj = new object(); public readonly Channel EventChannel = Channel.CreateBounded(1); - private FeatureProviderReference _defaultProvider; - private readonly Dictionary _namedProviderReferences = new Dictionary(); - private readonly List _activeSubscriptions = new List(); - private readonly SemaphoreSlim _shutdownSemaphore = new SemaphoreSlim(0); - - private ShutdownDelegate _shutdownDelegate; + private FeatureProvider _defaultProvider; + private readonly Dictionary _namedProviderReferences = new Dictionary(); + private readonly List _activeSubscriptions = new List(); private readonly Dictionary> _apiHandlers = new Dictionary>(); private readonly Dictionary>> _clientHandlers = new Dictionary>>(); @@ -32,11 +26,12 @@ internal class EventExecutor public EventExecutor() { this.Logger = new Logger(new NullLoggerFactory()); - this._shutdownDelegate = this.SignalShutdownAsync; var eventProcessing = new Thread(this.ProcessEventAsync); eventProcessing.Start(); } + public ValueTask DisposeAsync() => new(this.Shutdown()); + internal void AddApiLevelHandler(ProviderEventTypes eventType, EventHandlerDelegate handler) { lock (this._lockObj) @@ -114,7 +109,7 @@ internal void RegisterDefaultFeatureProvider(FeatureProvider provider) { var oldProvider = this._defaultProvider; - this._defaultProvider = new FeatureProviderReference(provider); + this._defaultProvider = provider; this.StartListeningAndShutdownOld(this._defaultProvider, oldProvider); } @@ -128,8 +123,8 @@ internal void RegisterClientFeatureProvider(string client, FeatureProvider provi } lock (this._lockObj) { - var newProvider = new FeatureProviderReference(provider); - FeatureProviderReference oldProvider = null; + var newProvider = provider; + FeatureProvider oldProvider = null; if (this._namedProviderReferences.TryGetValue(client, out var foundOldProvider)) { oldProvider = foundOldProvider; @@ -141,7 +136,7 @@ internal void RegisterClientFeatureProvider(string client, FeatureProvider provi } } - private void StartListeningAndShutdownOld(FeatureProviderReference newProvider, FeatureProviderReference oldProvider) + private void StartListeningAndShutdownOld(FeatureProvider newProvider, FeatureProvider oldProvider) { // check if the provider is already active - if not, we need to start listening for its emitted events if (!this.IsProviderActive(newProvider)) @@ -154,15 +149,11 @@ private void StartListeningAndShutdownOld(FeatureProviderReference newProvider, if (oldProvider != null && !this.IsProviderBound(oldProvider)) { this._activeSubscriptions.Remove(oldProvider); - var channel = oldProvider.Provider.GetEventChannel(); - if (channel != null) - { - channel.Writer.WriteAsync(new ShutdownSignal()); - } + oldProvider.GetEventChannel()?.Writer.Complete(); } } - private bool IsProviderBound(FeatureProviderReference provider) + private bool IsProviderBound(FeatureProvider provider) { if (this._defaultProvider == provider) { @@ -178,18 +169,18 @@ private bool IsProviderBound(FeatureProviderReference provider) return false; } - private bool IsProviderActive(FeatureProviderReference providerRef) + private bool IsProviderActive(FeatureProvider providerRef) { return this._activeSubscriptions.Contains(providerRef); } - private void EmitOnRegistration(FeatureProviderReference provider, ProviderEventTypes eventType, EventHandlerDelegate handler) + private void EmitOnRegistration(FeatureProvider provider, ProviderEventTypes eventType, EventHandlerDelegate handler) { if (provider == null) { return; } - var status = provider.Provider.GetStatus(); + var status = provider.GetStatus(); var message = ""; if (status == ProviderStatus.Ready && eventType == ProviderEventTypes.ProviderReady) @@ -211,7 +202,7 @@ private void EmitOnRegistration(FeatureProviderReference provider, ProviderEvent { handler.Invoke(new ProviderEventPayload { - ProviderName = provider.Provider?.GetMetadata()?.Name, + ProviderName = provider.GetMetadata()?.Name, Type = eventType, Message = message }); @@ -225,23 +216,22 @@ private void EmitOnRegistration(FeatureProviderReference provider, ProviderEvent private async void ProcessFeatureProviderEventsAsync(object providerRef) { - while (true) + var typedProviderRef = (FeatureProvider)providerRef; + if (typedProviderRef.GetEventChannel() is not { Reader: { } reader }) { - var typedProviderRef = (FeatureProviderReference)providerRef; - if (typedProviderRef.Provider.GetEventChannel() == null) - { - return; - } - var item = await typedProviderRef.Provider.GetEventChannel().Reader.ReadAsync().ConfigureAwait(false); + return; + } + + while (await reader.WaitToReadAsync().ConfigureAwait(false)) + { + if (!reader.TryRead(out var item)) + continue; switch (item) { case ProviderEventPayload eventPayload: await this.EventChannel.Writer.WriteAsync(new Event { Provider = typedProviderRef, EventPayload = eventPayload }).ConfigureAwait(false); break; - case ShutdownSignal _: - typedProviderRef.ShutdownSemaphore.Release(); - return; } } } @@ -249,9 +239,10 @@ private async void ProcessFeatureProviderEventsAsync(object providerRef) // Method to process events private async void ProcessEventAsync() { - while (true) + while (await this.EventChannel.Reader.WaitToReadAsync().ConfigureAwait(false)) { - var item = await this.EventChannel.Reader.ReadAsync().ConfigureAwait(false); + if (!this.EventChannel.Reader.TryRead(out var item)) + continue; switch (item) { @@ -307,9 +298,6 @@ private async void ProcessEventAsync() } } break; - case ShutdownSignal _: - this._shutdownSemaphore.Release(); - return; } } @@ -329,43 +317,15 @@ private void InvokeEventHandler(EventHandlerDelegate eventHandler, Event e) public async Task Shutdown() { - await this._shutdownDelegate().ConfigureAwait(false); - } + this.EventChannel.Writer.Complete(); - internal void SetShutdownDelegate(ShutdownDelegate del) - { - this._shutdownDelegate = del; - } - - // Method to signal shutdown - private async Task SignalShutdownAsync() - { - // Enqueue a shutdown signal - await this.EventChannel.Writer.WriteAsync(new ShutdownSignal()).ConfigureAwait(false); - - // Wait for the processing loop to acknowledge the shutdown - await this._shutdownSemaphore.WaitAsync().ConfigureAwait(false); - } - } - - internal class ShutdownSignal - { - } - - internal class FeatureProviderReference - { - internal readonly SemaphoreSlim ShutdownSemaphore = new SemaphoreSlim(0); - internal FeatureProvider Provider { get; } - - public FeatureProviderReference(FeatureProvider provider) - { - this.Provider = provider; + await this.EventChannel.Reader.Completion.ConfigureAwait(false); } } internal class Event { - internal FeatureProviderReference Provider { get; set; } + internal FeatureProvider Provider { get; set; } internal ProviderEventPayload EventPayload { get; set; } } } diff --git a/src/OpenFeature/OpenFeature.csproj b/src/OpenFeature/OpenFeature.csproj index 5429f43b..a6a5828f 100644 --- a/src/OpenFeature/OpenFeature.csproj +++ b/src/OpenFeature/OpenFeature.csproj @@ -7,6 +7,8 @@ + + diff --git a/src/OpenFeature/OpenFeatureClient.cs b/src/OpenFeature/OpenFeatureClient.cs index d979dae1..56fc518f 100644 --- a/src/OpenFeature/OpenFeatureClient.cs +++ b/src/OpenFeature/OpenFeatureClient.cs @@ -96,13 +96,13 @@ public FeatureClient(string name, string version, ILogger logger = null, Evaluat /// public void AddHandler(ProviderEventTypes eventType, EventHandlerDelegate handler) { - Api.Instance.EventExecutor.AddClientHandler(this._metadata.Name, eventType, handler); + Api.Instance.AddClientHandler(this._metadata.Name, eventType, handler); } /// public void RemoveHandler(ProviderEventTypes type, EventHandlerDelegate handler) { - Api.Instance.EventExecutor.RemoveClientHandler(this._metadata.Name, type, handler); + Api.Instance.RemoveClientHandler(this._metadata.Name, type, handler); } /// diff --git a/src/OpenFeature/ProviderRepository.cs b/src/OpenFeature/ProviderRepository.cs index ab2bdb30..f95d805c 100644 --- a/src/OpenFeature/ProviderRepository.cs +++ b/src/OpenFeature/ProviderRepository.cs @@ -12,7 +12,7 @@ namespace OpenFeature /// /// This class manages the collection of providers, both default and named, contained by the API. /// - internal sealed class ProviderRepository + internal sealed class ProviderRepository : IAsyncDisposable { private FeatureProvider _defaultProvider = new NoOpFeatureProvider(); @@ -31,6 +31,14 @@ internal sealed class ProviderRepository /// of that provider under different names.. private readonly ReaderWriterLockSlim _providersLock = new ReaderWriterLockSlim(); + public async ValueTask DisposeAsync() + { + using (this._providersLock) + { + await this.Shutdown().ConfigureAwait(false); + } + } + /// /// Set the default provider /// diff --git a/test/OpenFeature.Tests/OpenFeatureTests.cs b/test/OpenFeature.Tests/OpenFeatureTests.cs index 3e8b4d3d..28ba9f42 100644 --- a/test/OpenFeature.Tests/OpenFeatureTests.cs +++ b/test/OpenFeature.Tests/OpenFeatureTests.cs @@ -11,11 +11,6 @@ namespace OpenFeature.Tests { public class OpenFeatureTests : ClearOpenFeatureInstanceFixture { - static async Task EmptyShutdown() - { - await Task.FromResult(0).ConfigureAwait(false); - } - [Fact] [Specification("1.1.1", "The `API`, and any state it maintains SHOULD exist as a global singleton, even in cases wherein multiple versions of the `API` are present at runtime.")] public void OpenFeature_Should_Be_Singleton() @@ -79,9 +74,6 @@ public async Task OpenFeature_Should_Shutdown_Unused_Provider() [Specification("1.6.1", "The API MUST define a mechanism to propagate a shutdown request to active providers.")] public async Task OpenFeature_Should_Support_Shutdown() { - // configure the shutdown method of the event executor to do nothing - // to prevent eventing tests from failing - Api.Instance.EventExecutor.SetShutdownDelegate(EmptyShutdown); var providerA = Substitute.For(); providerA.GetStatus().Returns(ProviderStatus.NotReady);