diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index 6d0d8feb4..02c1edf25 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java @@ -339,12 +339,23 @@ public void clearHooks() { * Once shut down is complete, API is reset and ready to use again. */ public void shutdown() { + List managersToShutdown; try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { - providerRepository.shutdown(); + // Mark repository as shutting down while holding lock. + // This ensures setProvider calls will throw IllegalStateException. + managersToShutdown = providerRepository.prepareShutdown(); + } + + if (managersToShutdown != null) { + // Complete shutdown without holding lock to avoid deadlock. + // Pending tasks (e.g., initializeProvider) may need the read lock to emit events. + providerRepository.completeShutdown(managersToShutdown); eventSupport.shutdown(); - providerRepository = new ProviderRepository(this); - eventSupport = new EventSupport(); + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { + providerRepository = new ProviderRepository(this); + eventSupport = new EventSupport(); + } } } diff --git a/src/main/java/dev/openfeature/sdk/ProviderRepository.java b/src/main/java/dev/openfeature/sdk/ProviderRepository.java index 147074a58..e704d7ed1 100644 --- a/src/main/java/dev/openfeature/sdk/ProviderRepository.java +++ b/src/main/java/dev/openfeature/sdk/ProviderRepository.java @@ -10,6 +10,8 @@ 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.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -23,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(); @@ -162,6 +165,9 @@ private void prepareAndInitializeProvider( final FeatureProviderStateManager oldStateManager; synchronized (registerStateManagerLock) { + if (isShuttingDown.get()) { + throw new IllegalStateException("Provider cannot be set while repository is shutting down"); + } FeatureProviderStateManager existing = getExistingStateManagerForProvider(newProvider); if (existing == null) { newStateManager = new FeatureProviderStateManager(newProvider); @@ -228,9 +234,11 @@ private void initializeProvider( } private void shutDownOld(FeatureProviderStateManager oldManager, Consumer afterShutdown) { - if (oldManager != null && !isStateManagerRegistered(oldManager)) { - shutdownProvider(oldManager); - afterShutdown.accept(oldManager.getProvider()); + synchronized (registerStateManagerLock) { + if (oldManager != null && !isStateManagerRegistered(oldManager)) { + shutdownProvider(oldManager); + afterShutdown.accept(oldManager.getProvider()); + } } } @@ -254,16 +262,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); } - }); + } } /** @@ -272,10 +291,52 @@ private void shutdownProvider(FeatureProvider provider) { * including the default feature provider. */ public void shutdown() { - Stream.concat(Stream.of(this.defaultStateManger.get()), this.stateManagers.values().stream()) - .distinct() - .forEach(this::shutdownProvider); - this.stateManagers.clear(); + List managersToShutdown = prepareShutdown(); + if (managersToShutdown != null) { + completeShutdown(managersToShutdown); + } + } + + /** + * Prepares the repository for shutdown by marking it as shutting down and + * collecting all managers that need to be shut down. + * + *

After this call, any attempt to set a provider will throw IllegalStateException. + * + * @return list of managers to shut down, or null if shutdown was already initiated + */ + List prepareShutdown() { + synchronized (registerStateManagerLock) { + if (isShuttingDown.getAndSet(true)) { + return null; + } + + List managersToShutdown = Stream.concat( + Stream.of(this.defaultStateManger.get()), this.stateManagers.values().stream()) + .distinct() + .collect(Collectors.toList()); + this.stateManagers.clear(); + return managersToShutdown; + } + } + + /** + * Completes the shutdown by shutting down all providers and waiting for + * pending tasks to complete. + * + * @param managersToShutdown the managers to shut down (from prepareShutdown) + */ + void completeShutdown(List managersToShutdown) { + managersToShutdown.forEach(this::shutdownProvider); 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..c9d69ad73 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; @@ -15,6 +16,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 +291,199 @@ 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()); + } + + // 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") + 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 + @DisplayName("should handle shutdown during provider initialization") + void shouldHandleShutdownDuringProviderInitialization() throws Exception { + FeatureProvider slowInitProvider = createMockedProvider(); + AtomicBoolean shutdownCalled = new AtomicBoolean(false); + + doDelayResponse(Duration.ofMillis(500)).when(slowInitProvider).initialize(any()); + + doAnswer(invocation -> { + shutdownCalled.set(true); + return null; + }) + .when(slowInitProvider) + .shutdown(); + + providerRepository.setProvider( + slowInitProvider, + mockAfterSet(), + mockAfterInit(), + mockAfterShutdown(), + mockAfterError(), + false); + + // 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(() -> setFeatureProvider(newProvider)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("shutting down"); + } + + @Test + @DisplayName("prepareShutdown should return null on second call") + void prepareShutdownShouldReturnNullOnSecondCall() { + FeatureProvider provider = createMockedProvider(); + setFeatureProvider(provider); + + // First call should return managers list + var managers = providerRepository.prepareShutdown(); + assertThat(managers).isNotNull(); + assertThat(managers).isNotEmpty(); + + // Second call should be a no-op and return null (already shutting down) + var secondResult = providerRepository.prepareShutdown(); + assertThat(secondResult).isNull(); + } + + @Test + @DisplayName("should fall back to direct shutdown when executor rejects tasks") + void shouldFallBackToDirectShutdownWhenExecutorRejectsTasks() throws Exception { + FeatureProvider oldProvider = createMockedProvider(); + FeatureProvider newProvider = createMockedProvider(); + AtomicBoolean initializationStarted = new AtomicBoolean(false); + AtomicBoolean proceedWithInit = new AtomicBoolean(false); + + // Make oldProvider's initialization block until we signal + doAnswer(invocation -> { + initializationStarted.set(true); + while (!proceedWithInit.get()) { + Thread.sleep(10); + } + return null; + }) + .when(oldProvider) + .initialize(any()); + + // Start async initialization (will block) + providerRepository.setProvider( + oldProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); + + // Wait for initialization to start + await().atMost(Duration.ofSeconds(1)).untilTrue(initializationStarted); + + // Now set a new provider - this will trigger shutDownOld for oldProvider + // after initialization completes, but we haven't completed init yet + providerRepository.setProvider( + newProvider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); + + // Call shutdown on repository - this will shutdown the executor + var managers = providerRepository.prepareShutdown(); + providerRepository.completeShutdown(managers); + + // Now let the initialization complete - shutDownOld will be called but executor is shutdown + // This triggers the RejectedExecutionException path which falls back to direct shutdown + proceedWithInit.set(true); + + // Both providers should eventually be shut down (oldProvider via direct call due to + // RejectedExecutionException) + verify(oldProvider, timeout(TIMEOUT)).shutdown(); + verify(newProvider, timeout(TIMEOUT)).shutdown(); + } + } } @Test diff --git a/src/test/java/dev/openfeature/sdk/ShutdownBehaviorSpecTest.java b/src/test/java/dev/openfeature/sdk/ShutdownBehaviorSpecTest.java index 1bb7d4b62..2c6a6304e 100644 --- a/src/test/java/dev/openfeature/sdk/ShutdownBehaviorSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/ShutdownBehaviorSpecTest.java @@ -142,5 +142,19 @@ void apiIsReadyToUseAfterShutdown() { NoOpProvider p2 = new NoOpProvider(); api.setProvider(p2); } + + @Test + @DisplayName("calling shutdown twice should be safe and idempotent") + void callingShutdownTwiceShouldBeSafe() { + FeatureProvider provider = ProviderFixture.createMockedProvider(); + setFeatureProvider(provider); + + api.shutdown(); + + // Second shutdown should be a no-op (no exception, provider not called twice) + api.shutdown(); + + verify(provider, times(1)).shutdown(); + } } } diff --git a/src/test/java/dev/openfeature/sdk/vmlens/ProviderRepositoryCT.java b/src/test/java/dev/openfeature/sdk/vmlens/ProviderRepositoryCT.java new file mode 100644 index 000000000..2d88f698f --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/vmlens/ProviderRepositoryCT.java @@ -0,0 +1,158 @@ +package dev.openfeature.sdk.vmlens; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.vmlens.api.AllInterleavings; +import com.vmlens.api.Runner; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.OpenFeatureAPI; +import dev.openfeature.sdk.OpenFeatureAPITestUtil; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; + +/** + * Concurrency tests for ProviderRepository shutdown behavior using VMLens. + * + *

These tests verify that concurrent shutdown operations are safe and produce + * consistent results regardless of thread interleaving. Tests operate through + * the public OpenFeatureAPI since ProviderRepository is package-private. + */ +class ProviderRepositoryCT { + + private FeatureProvider createMockedProvider(String name, AtomicInteger shutdownCounter) throws Exception { + FeatureProvider provider = mock(FeatureProvider.class); + when(provider.getMetadata()).thenReturn(() -> name); + doAnswer(invocation -> { + shutdownCounter.incrementAndGet(); + return null; + }) + .when(provider) + .shutdown(); + doAnswer(invocation -> null).when(provider).initialize(any()); + return provider; + } + + @Test + void concurrentShutdown_providerShutdownCalledExactlyOnce() throws Exception { + try (AllInterleavings allInterleavings = + new AllInterleavings("Concurrent API shutdown - provider called once")) { + while (allInterleavings.hasNext()) { + // Fresh state for each interleaving + AtomicInteger shutdownCount = new AtomicInteger(0); + FeatureProvider provider = createMockedProvider("test-provider", shutdownCount); + OpenFeatureAPI api = OpenFeatureAPITestUtil.createAPI(); + + // Set provider and wait for initialization to complete + api.setProviderAndWait(provider); + + // Run concurrent shutdowns through the public API + Runner.runParallel(api::shutdown, api::shutdown, api::shutdown); + + // INVARIANT: Provider shutdown must be called exactly once + assertThat(shutdownCount.get()) + .as("Provider.shutdown() should be called exactly once regardless of thread interleaving") + .isEqualTo(1); + } + } + } + + @Test + void concurrentShutdown_providerShutdownCalledExactlyOnce_duringPendingInit() throws Exception { + try (AllInterleavings allInterleavings = new AllInterleavings("Shutdown during pending initialization")) { + while (allInterleavings.hasNext()) { + AtomicInteger shutdownCount = new AtomicInteger(0); + FeatureProvider provider = createMockedProvider("test-provider", shutdownCount); + OpenFeatureAPI api = OpenFeatureAPITestUtil.createAPI(); + + // Set provider without waiting - initialization task is pending on executor + api.setProvider(provider); + + // Shutdown while init task may still be pending + api.shutdown(); + + assertThat(shutdownCount.get()) + .as("Provider.shutdown() should be called exactly once") + .isEqualTo(1); + } + } + } + + @Test + void setProviderDuringShutdown_eitherSucceedsOrThrows() throws Exception { + try (AllInterleavings allInterleavings = new AllInterleavings("setProvider racing with shutdown")) { + while (allInterleavings.hasNext()) { + // Fresh state for each interleaving + AtomicInteger provider1ShutdownCount = new AtomicInteger(0); + AtomicInteger provider2ShutdownCount = new AtomicInteger(0); + FeatureProvider provider1 = createMockedProvider("provider-1", provider1ShutdownCount); + FeatureProvider provider2 = createMockedProvider("provider-2", provider2ShutdownCount); + OpenFeatureAPI api = OpenFeatureAPITestUtil.createAPI(); + + // Set initial provider + api.setProviderAndWait(provider1); + + // Track outcomes + AtomicInteger setProviderSucceeded = new AtomicInteger(0); + AtomicInteger setProviderFailed = new AtomicInteger(0); + + Runner.runParallel(api::shutdown, () -> { + try { + api.setProvider(provider2); + setProviderSucceeded.incrementAndGet(); + } catch (IllegalStateException e) { + if (e.getMessage().contains("shutting down")) { + setProviderFailed.incrementAndGet(); + } else { + throw e; + } + } + }); + + // INVARIANT: setProvider must have exactly one outcome + int totalOutcomes = setProviderSucceeded.get() + setProviderFailed.get(); + assertThat(totalOutcomes) + .as("setProvider must have exactly one outcome (success or failure)") + .isEqualTo(1); + + // INVARIANT: Original provider should always be shut down + assertThat(provider1ShutdownCount.get()) + .as("Original provider should be shut down exactly once") + .isEqualTo(1); + } + } + } + + @Test + void concurrentShutdown_multipleProvidersShutdownExactlyOnce() throws Exception { + try (AllInterleavings allInterleavings = new AllInterleavings("Concurrent shutdown - multiple providers")) { + while (allInterleavings.hasNext()) { + AtomicInteger provider1ShutdownCount = new AtomicInteger(0); + AtomicInteger provider2ShutdownCount = new AtomicInteger(0); + + FeatureProvider provider1 = createMockedProvider("provider-1", provider1ShutdownCount); + FeatureProvider provider2 = createMockedProvider("provider-2", provider2ShutdownCount); + + OpenFeatureAPI api = OpenFeatureAPITestUtil.createAPI(); + + // Register providers to named domains + api.setProviderAndWait("domain-1", provider1); + api.setProviderAndWait("domain-2", provider2); + + // Run concurrent shutdowns + Runner.runParallel(api::shutdown, api::shutdown); + + // INVARIANT: Each provider shut down exactly once + assertThat(provider1ShutdownCount.get()) + .as("Provider 1 shutdown count") + .isEqualTo(1); + assertThat(provider2ShutdownCount.get()) + .as("Provider 2 shutdown count") + .isEqualTo(1); + } + } + } +}