From 9efbc78051c7e0c8cf2781f4e48fa7c532f7e741 Mon Sep 17 00:00:00 2001 From: Nicklas Lundin Date: Tue, 25 Nov 2025 13:21:10 +0100 Subject: [PATCH 1/4] fix: allow for providers to safely shutdown Signed-off-by: Nicklas Lundin --- .../openfeature/sdk/ProviderRepository.java | 10 +++ .../sdk/ProviderRepositoryTest.java | 75 +++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/src/main/java/dev/openfeature/sdk/ProviderRepository.java b/src/main/java/dev/openfeature/sdk/ProviderRepository.java index 147074a58..356704f98 100644 --- a/src/main/java/dev/openfeature/sdk/ProviderRepository.java +++ b/src/main/java/dev/openfeature/sdk/ProviderRepository.java @@ -10,6 +10,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -277,5 +278,14 @@ public void shutdown() { .forEach(this::shutdownProvider); this.stateManagers.clear(); taskExecutor.shutdown(); + try { + if (!taskExecutor.awaitTermination(3, TimeUnit.SECONDS)) { + log.warn("Task executor did not terminate before the timeout period had elapsed"); + taskExecutor.shutdownNow(); + } + } catch (InterruptedException e) { + taskExecutor.shutdownNow(); + Thread.currentThread().interrupt(); + } } } diff --git a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java index 7041df5c1..99e34436c 100644 --- a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java +++ b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java @@ -15,6 +15,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; @@ -289,6 +290,80 @@ void shouldRunLambdasOnError() throws Exception { verify(afterError, timeout(TIMEOUT)).accept(eq(errorFeatureProvider), any()); } } + + @Nested + class GracefulShutdownBehavior { + + @Test + @DisplayName("should complete shutdown successfully when executor terminates within timeout") + void shouldCompleteShutdownSuccessfullyWhenExecutorTerminatesWithinTimeout() { + FeatureProvider provider = createMockedProvider(); + setFeatureProvider(provider); + + assertThatCode(() -> providerRepository.shutdown()).doesNotThrowAnyException(); + + verify(provider, timeout(TIMEOUT)).shutdown(); + } + + @Test + @DisplayName("should force shutdown when executor does not terminate within timeout") + void shouldForceShutdownWhenExecutorDoesNotTerminateWithinTimeout() throws Exception { + FeatureProvider provider = createMockedProvider(); + AtomicBoolean wasInterrupted = new AtomicBoolean(false); + doAnswer(invocation -> { + try { + Thread.sleep(TIMEOUT); + } catch (InterruptedException e) { + wasInterrupted.set(true); + throw e; + } + return null; + }) + .when(provider) + .shutdown(); + + setFeatureProvider(provider); + + assertThatCode(() -> providerRepository.shutdown()).doesNotThrowAnyException(); + + verify(provider, timeout(TIMEOUT)).shutdown(); + // Verify that shutdownNow() interrupted the running shutdown task + await().atMost(Duration.ofSeconds(1)) + .untilAsserted(() -> assertThat(wasInterrupted.get()).isTrue()); + } + + @Test + @DisplayName("should handle interruption during shutdown gracefully") + void shouldHandleInterruptionDuringShutdownGracefully() throws Exception { + FeatureProvider provider = createMockedProvider(); + setFeatureProvider(provider); + + Thread shutdownThread = new Thread(() -> { + providerRepository.shutdown(); + }); + + shutdownThread.start(); + shutdownThread.interrupt(); + shutdownThread.join(TIMEOUT); + + assertThat(shutdownThread.isAlive()).isFalse(); + verify(provider, timeout(TIMEOUT)).shutdown(); + } + + @Test + @DisplayName("should not hang indefinitely on shutdown") + void shouldNotHangIndefinitelyOnShutdown() { + FeatureProvider provider = createMockedProvider(); + setFeatureProvider(provider); + + await().alias("shutdown should complete within reasonable time") + .atMost(Duration.ofSeconds(5)) + .until(() -> { + providerRepository.shutdown(); + return true; + }); + } + } } @Test From dc340c178cf87b9cecdd80df7bbd68006e8a91a7 Mon Sep 17 00:00:00 2001 From: Nicklas Lundin Date: Thu, 27 Nov 2025 13:34:45 +0100 Subject: [PATCH 2/4] fix: prevent race conditions during repository shutdown Signed-off-by: Nicklas Lundin --- .../openfeature/sdk/ProviderRepository.java | 29 +++++- .../sdk/ProviderRepositoryTest.java | 99 +++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/ProviderRepository.java b/src/main/java/dev/openfeature/sdk/ProviderRepository.java index 356704f98..31d3b0a56 100644 --- a/src/main/java/dev/openfeature/sdk/ProviderRepository.java +++ b/src/main/java/dev/openfeature/sdk/ProviderRepository.java @@ -11,6 +11,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -24,6 +25,7 @@ class ProviderRepository { private final Map stateManagers = new ConcurrentHashMap<>(); private final AtomicReference defaultStateManger = new AtomicReference<>(new FeatureProviderStateManager(new NoOpProvider())); + private final AtomicBoolean isShuttingDown = new AtomicBoolean(false); private final ExecutorService taskExecutor = Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-provider-thread", true)); private final Object registerStateManagerLock = new Object(); @@ -159,6 +161,10 @@ private void prepareAndInitializeProvider( Consumer afterShutdown, BiConsumer afterError, boolean waitForInit) { + if (isShuttingDown.get()) { + throw new IllegalStateException("Provider cannot be set while repository is shutting down"); + } + final FeatureProviderStateManager newStateManager; final FeatureProviderStateManager oldStateManager; @@ -255,16 +261,27 @@ private void shutdownProvider(FeatureProviderStateManager manager) { } private void shutdownProvider(FeatureProvider provider) { - taskExecutor.submit(() -> { + try { + taskExecutor.submit(() -> { + try { + provider.shutdown(); + } catch (Exception e) { + log.error( + "Exception when shutting down feature provider {}", + provider.getClass().getName(), + e); + } + }); + } catch (java.util.concurrent.RejectedExecutionException e) { try { provider.shutdown(); - } catch (Exception e) { + } catch (Exception ex) { log.error( "Exception when shutting down feature provider {}", provider.getClass().getName(), - e); + ex); } - }); + } } /** @@ -273,6 +290,10 @@ private void shutdownProvider(FeatureProvider provider) { * including the default feature provider. */ public void shutdown() { + if (isShuttingDown.getAndSet(true)) { + return; + } + Stream.concat(Stream.of(this.defaultStateManger.get()), this.stateManagers.values().stream()) .distinct() .forEach(this::shutdownProvider); diff --git a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java index 99e34436c..c92ead163 100644 --- a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java +++ b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java @@ -4,6 +4,7 @@ import static dev.openfeature.sdk.testutils.stubbing.ConditionStubber.doDelayResponse; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.awaitility.Awaitility.await; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -363,6 +364,104 @@ void shouldNotHangIndefinitelyOnShutdown() { return true; }); } + + @Test + @DisplayName("should handle shutdown during provider initialization") + void shouldHandleShutdownDuringProviderInitialization() throws Exception { + FeatureProvider slowInitProvider = createMockedProvider(); + AtomicBoolean initStarted = new AtomicBoolean(false); + AtomicBoolean shutdownCalled = new AtomicBoolean(false); + + doAnswer(invocation -> { + initStarted.set(true); + Thread.sleep(500); + return null; + }) + .when(slowInitProvider) + .initialize(any()); + + doAnswer(invocation -> { + shutdownCalled.set(true); + return null; + }) + .when(slowInitProvider) + .shutdown(); + + providerRepository.setProvider(slowInitProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); + + await().atMost(Duration.ofSeconds(1)).untilTrue(initStarted); + + // Call shutdown while initialization is in progress + assertThatCode(() -> providerRepository.shutdown()).doesNotThrowAnyException(); + + await().atMost(Duration.ofSeconds(1)).untilTrue(shutdownCalled); + verify(slowInitProvider, times(1)).shutdown(); + } + + @Test + @DisplayName("should handle provider replacement during shutdown") + void shouldHandleProviderReplacementDuringShutdown() throws Exception { + FeatureProvider oldProvider = createMockedProvider(); + FeatureProvider newProvider = createMockedProvider(); + AtomicBoolean oldProviderShutdownCalled = new AtomicBoolean(false); + + doAnswer(invocation -> { + oldProviderShutdownCalled.set(true); + return null; + }) + .when(oldProvider) + .shutdown(); + + providerRepository.setProvider(oldProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), true); + + // Replace provider (this will trigger old provider shutdown in background) + providerRepository.setProvider(newProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); + + assertThatCode(() -> providerRepository.shutdown()).doesNotThrowAnyException(); + + await().atMost(Duration.ofSeconds(1)).untilTrue(oldProviderShutdownCalled); + verify(oldProvider, times(1)).shutdown(); + verify(newProvider, times(1)).shutdown(); + } + + @Test + @DisplayName("should prevent adding providers after shutdown has started") + void shouldPreventAddingProvidersAfterShutdownHasStarted() { + FeatureProvider provider = createMockedProvider(); + setFeatureProvider(provider); + + providerRepository.shutdown(); + + FeatureProvider newProvider = createMockedProvider(); + assertThatThrownBy(() -> providerRepository.setProvider( + newProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("shutting down"); + } + + @Test + @DisplayName("should handle concurrent shutdown calls gracefully") + void shouldHandleConcurrentShutdownCallsGracefully() { + FeatureProvider provider = createMockedProvider(); + setFeatureProvider(provider); + + assertThatCode(() -> { + Thread t1 = new Thread(() -> providerRepository.shutdown()); + Thread t2 = new Thread(() -> providerRepository.shutdown()); + Thread t3 = new Thread(() -> providerRepository.shutdown()); + + t1.start(); + t2.start(); + t3.start(); + + t1.join(TIMEOUT); + t2.join(TIMEOUT); + t3.join(TIMEOUT); + }) + .doesNotThrowAnyException(); + + verify(provider, times(1)).shutdown(); + } } } From 6ac560cd542c5f282d929c49d3154c9717bc5e9f Mon Sep 17 00:00:00 2001 From: Nicklas Lundin Date: Thu, 27 Nov 2025 13:40:17 +0100 Subject: [PATCH 3/4] fixup! fix: prevent race conditions during repository shutdown Signed-off-by: Nicklas Lundin --- .../sdk/ProviderRepositoryTest.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java index c92ead163..a4e664bc5 100644 --- a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java +++ b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java @@ -387,7 +387,13 @@ void shouldHandleShutdownDuringProviderInitialization() throws Exception { .when(slowInitProvider) .shutdown(); - providerRepository.setProvider(slowInitProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); + providerRepository.setProvider( + slowInitProvider, + mockAfterSet(), + mockAfterInit(), + mockAfterShutdown(), + mockAfterError(), + false); await().atMost(Duration.ofSeconds(1)).untilTrue(initStarted); @@ -412,10 +418,12 @@ void shouldHandleProviderReplacementDuringShutdown() throws Exception { .when(oldProvider) .shutdown(); - providerRepository.setProvider(oldProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), true); + providerRepository.setProvider( + oldProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), true); // Replace provider (this will trigger old provider shutdown in background) - providerRepository.setProvider(newProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); + providerRepository.setProvider( + newProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); assertThatCode(() -> providerRepository.shutdown()).doesNotThrowAnyException(); @@ -434,7 +442,12 @@ void shouldPreventAddingProvidersAfterShutdownHasStarted() { FeatureProvider newProvider = createMockedProvider(); assertThatThrownBy(() -> providerRepository.setProvider( - newProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false)) + newProvider, + mockAfterSet(), + mockAfterInit(), + mockAfterShutdown(), + mockAfterError(), + false)) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("shutting down"); } From f6126878b21c1c770d0ac9b4ce61e8f60038c283 Mon Sep 17 00:00:00 2001 From: Nicklas Lundin Date: Fri, 23 Jan 2026 16:12:45 +0100 Subject: [PATCH 4/4] fixup! fix: prevent race conditions during repository shutdown Remove flaky concurrency tests; proper testing via VMLens. Co-Authored-By: Claude Opus 4.5 --- .../sdk/ProviderRepositoryTest.java | 65 +++---------------- 1 file changed, 8 insertions(+), 57 deletions(-) diff --git a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java index a4e664bc5..dfe8e8074 100644 --- a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java +++ b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java @@ -333,23 +333,9 @@ void shouldForceShutdownWhenExecutorDoesNotTerminateWithinTimeout() throws Excep .untilAsserted(() -> assertThat(wasInterrupted.get()).isTrue()); } - @Test - @DisplayName("should handle interruption during shutdown gracefully") - void shouldHandleInterruptionDuringShutdownGracefully() throws Exception { - FeatureProvider provider = createMockedProvider(); - setFeatureProvider(provider); - - Thread shutdownThread = new Thread(() -> { - providerRepository.shutdown(); - }); - - shutdownThread.start(); - shutdownThread.interrupt(); - shutdownThread.join(TIMEOUT); - - assertThat(shutdownThread.isAlive()).isFalse(); - verify(provider, timeout(TIMEOUT)).shutdown(); - } + // Note: shouldHandleInterruptionDuringShutdownGracefully was removed because the + // interrupt timing is not guaranteed. Proper concurrency testing is done in + // ProviderRepositoryCT using VMLens. @Test @DisplayName("should not hang indefinitely on shutdown") @@ -369,16 +355,9 @@ void shouldNotHangIndefinitelyOnShutdown() { @DisplayName("should handle shutdown during provider initialization") void shouldHandleShutdownDuringProviderInitialization() throws Exception { FeatureProvider slowInitProvider = createMockedProvider(); - AtomicBoolean initStarted = new AtomicBoolean(false); AtomicBoolean shutdownCalled = new AtomicBoolean(false); - doAnswer(invocation -> { - initStarted.set(true); - Thread.sleep(500); - return null; - }) - .when(slowInitProvider) - .initialize(any()); + doDelayResponse(Duration.ofMillis(500)).when(slowInitProvider).initialize(any()); doAnswer(invocation -> { shutdownCalled.set(true); @@ -395,8 +374,6 @@ void shouldHandleShutdownDuringProviderInitialization() throws Exception { mockAfterError(), false); - await().atMost(Duration.ofSeconds(1)).untilTrue(initStarted); - // Call shutdown while initialization is in progress assertThatCode(() -> providerRepository.shutdown()).doesNotThrowAnyException(); @@ -441,40 +418,14 @@ void shouldPreventAddingProvidersAfterShutdownHasStarted() { providerRepository.shutdown(); FeatureProvider newProvider = createMockedProvider(); - assertThatThrownBy(() -> providerRepository.setProvider( - newProvider, - mockAfterSet(), - mockAfterInit(), - mockAfterShutdown(), - mockAfterError(), - false)) + assertThatThrownBy(() -> setFeatureProvider(newProvider)) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("shutting down"); } - @Test - @DisplayName("should handle concurrent shutdown calls gracefully") - void shouldHandleConcurrentShutdownCallsGracefully() { - FeatureProvider provider = createMockedProvider(); - setFeatureProvider(provider); - - assertThatCode(() -> { - Thread t1 = new Thread(() -> providerRepository.shutdown()); - Thread t2 = new Thread(() -> providerRepository.shutdown()); - Thread t3 = new Thread(() -> providerRepository.shutdown()); - - t1.start(); - t2.start(); - t3.start(); - - t1.join(TIMEOUT); - t2.join(TIMEOUT); - t3.join(TIMEOUT); - }) - .doesNotThrowAnyException(); - - verify(provider, times(1)).shutdown(); - } + // Note: shouldHandleConcurrentShutdownCallsGracefully was removed because starting + // multiple threads doesn't guarantee parallel execution. Proper concurrency testing + // is done in ProviderRepositoryCT using VMLens which explores all thread interleavings. } }