-
Notifications
You must be signed in to change notification settings - Fork 54
feat: support isolated API instances #1928
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
base: main
Are you sure you want to change the base?
Changes from all commits
e7b91f3
b084f9c
3815faa
da9d472
7b9b39d
5f14bb9
c06e500
0e53150
586e636
2da34f5
3a25c3c
4041ebc
27ca0a2
3e7374c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,28 +9,63 @@ | |
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentLinkedQueue; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Consumer; | ||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| /** | ||
| * A global singleton which holds base configuration for the OpenFeature | ||
| * library. | ||
| * Configuration here will be shared across all {@link Client}s. | ||
| * Holds base configuration for the OpenFeature library. | ||
| * | ||
| * <p>Most applications should use the global singleton via {@link #getInstance()}; configuration | ||
| * there is shared across all {@link Client}s. For dependency-injection frameworks, testing, or | ||
| * multi-tenant scenarios that need fully independent state (providers, hooks, evaluation context, | ||
| * event handlers, transaction context propagators), instantiate a new instance directly with | ||
| * {@code new OpenFeatureAPI()}. | ||
| * | ||
| * <p><strong>Note:</strong> Isolated API instances (per spec section 1.8) are experimental and | ||
| * subject to change. | ||
| * | ||
| * @see <a href="https://openfeature.dev/specification/sections/flag-evaluation#18-isolated-api-instances"> | ||
| * Spec §1.8 — Isolated API Instances</a> | ||
| */ | ||
| @Slf4j | ||
| @SuppressWarnings("PMD.UnusedLocalVariable") | ||
| public class OpenFeatureAPI implements EventBus<OpenFeatureAPI> { | ||
| // package-private multi-read/single-write lock | ||
| static AutoCloseableReentrantReadWriteLock lock = new AutoCloseableReentrantReadWriteLock(); | ||
|
|
||
| /** | ||
| * Global registry tracking which API instance each provider is currently bound to. | ||
| * Used to detect violations of spec requirement 1.8.4 (a provider SHOULD NOT be | ||
| * registered with more than one API instance simultaneously). | ||
| */ | ||
| private static final ConcurrentHashMap<FeatureProvider, OpenFeatureAPI> GLOBAL_PROVIDER_REGISTRY = | ||
| new ConcurrentHashMap<>(); | ||
|
|
||
| // package-private multi-read/single-write lock (instance-level for isolation) | ||
| final AutoCloseableReentrantReadWriteLock lock; | ||
| private final ConcurrentLinkedQueue<Hook> apiHooks; | ||
| private ProviderRepository providerRepository; | ||
| private EventSupport eventSupport; | ||
| private final AtomicReference<EvaluationContext> evaluationContext = new AtomicReference<>(); | ||
| private TransactionContextPropagator transactionContextPropagator; | ||
|
|
||
| protected OpenFeatureAPI() { | ||
| /** | ||
| * Creates a new, independent {@link OpenFeatureAPI} instance with fully isolated state | ||
| * (providers, hooks, evaluation context, event handlers, transaction context propagators). | ||
| * | ||
| * <p>For typical usage, prefer the global singleton via {@link #getInstance()}. | ||
| * | ||
| * <p><strong>Note:</strong> Isolated API instances (per spec section 1.8) are experimental and | ||
| * subject to change. | ||
| */ | ||
| public OpenFeatureAPI() { | ||
|
Member
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 see the IIUC, a public constructor on |
||
| this(new AutoCloseableReentrantReadWriteLock()); | ||
| } | ||
|
|
||
| // Package-private constructor for testing with a custom lock. | ||
| OpenFeatureAPI(AutoCloseableReentrantReadWriteLock lock) { | ||
| this.lock = lock; | ||
| apiHooks = new ConcurrentLinkedQueue<>(); | ||
| providerRepository = new ProviderRepository(this); | ||
| eventSupport = new EventSupport(); | ||
|
|
@@ -251,7 +286,7 @@ public void setProviderAndWait(String domain, FeatureProvider provider) throws O | |
|
|
||
| private void attachEventProvider(FeatureProvider provider) { | ||
| if (provider instanceof EventProvider) { | ||
| ((EventProvider) provider).attach(this::runHandlersForProvider); | ||
| ((EventProvider) provider).attach(this::runHandlersForProvider, this.lock); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -332,6 +367,30 @@ public void clearHooks() { | |
| this.apiHooks.clear(); | ||
| } | ||
|
|
||
| /** | ||
| * Registers a provider with the global registry, warning if it is already | ||
| * bound to a different API instance (spec requirement 1.8.4). | ||
| */ | ||
| void registerGlobalProvider(FeatureProvider provider) { | ||
| GLOBAL_PROVIDER_REGISTRY.compute(provider, (p, existing) -> { | ||
| if (existing != null && existing != this) { | ||
| log.warn("Provider " | ||
| + provider.getClass().getName() | ||
| + " is already registered with another API instance. " | ||
| + "A provider SHOULD NOT be bound to more than one API instance " | ||
| + "simultaneously (spec requirement 1.8.4)."); | ||
| } | ||
| return this; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Removes the provider from the global registry if this instance is the current owner. | ||
| */ | ||
| void deregisterGlobalProvider(FeatureProvider provider) { | ||
| GLOBAL_PROVIDER_REGISTRY.remove(provider, this); | ||
| } | ||
|
|
||
| /** | ||
| * Shut down and reset the current status of OpenFeature API. | ||
| * This call cleans up all active providers and attempts to shut down internal | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| package dev.openfeature.sdk.isolated; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import dev.openfeature.sdk.FeatureProvider; | ||
| import dev.openfeature.sdk.ImmutableContext; | ||
| import dev.openfeature.sdk.NoOpTransactionContextPropagator; | ||
| import dev.openfeature.sdk.OpenFeatureAPI; | ||
| import dev.openfeature.sdk.ThreadLocalTransactionContextPropagator; | ||
| import dev.openfeature.sdk.providers.memory.InMemoryProvider; | ||
| import java.util.Map; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class IsolatedAPISingeltonTest { | ||
|
|
||
| private final OpenFeatureAPI singleton = OpenFeatureAPI.getInstance(); | ||
|
|
||
| @AfterEach | ||
| void restoreSingleton() { | ||
| singleton.shutdown(); | ||
| singleton.clearHooks(); | ||
| singleton.setEvaluationContext(null); | ||
| singleton.setTransactionContextPropagator(new NoOpTransactionContextPropagator()); | ||
| } | ||
|
|
||
| /** | ||
| * Requirement 1.8.1 — isolated instances do not share state with | ||
| * the global singleton. | ||
| */ | ||
| @Test | ||
| @DisplayName("isolated instance does not interfere with singleton") | ||
| void isolatedInstanceDoesNotInterfereWithSingleton() { | ||
|
Member
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. This tests that mutating an isolated instance doesn't affect the singleton, which is great. Worth adding the reverse direction too; e.g. set a provider/hook/context on the singleton and assert a previously created isolated instance is unaffected. |
||
| // record singleton baseline | ||
| FeatureProvider singletonProvider = singleton.getProvider(); | ||
|
|
||
| OpenFeatureAPI isolated = new OpenFeatureAPI(); | ||
| assertThat(isolated).isNotSameAs(singleton); | ||
|
|
||
| // mutate only the isolated instance | ||
| isolated.setProvider(new InMemoryProvider(Map.of())); | ||
| isolated.addHooks(new NoOpHook()); | ||
| isolated.setEvaluationContext(new ImmutableContext("isolated-key")); | ||
|
|
||
| // singleton remains at baseline | ||
| assertThat(singleton.getProvider()).isSameAs(singletonProvider); | ||
| assertThat(singleton.getHooks()).isEmpty(); | ||
| assertThat(singleton.getEvaluationContext()).isNull(); | ||
| } | ||
|
|
||
| /** | ||
| * Requirement 1.8.1 — mutating the singleton does not affect a | ||
| * previously created isolated instance. | ||
| */ | ||
| @Test | ||
| @DisplayName("singleton does not interfere with isolated instance") | ||
| void singletonDoesNotInterfereWithIsolatedInstance() { | ||
| OpenFeatureAPI isolated = new OpenFeatureAPI(); | ||
|
|
||
| // record isolated baseline | ||
| FeatureProvider isolatedProvider = isolated.getProvider(); | ||
|
|
||
| // mutate only the singleton | ||
| singleton.setProvider(new InMemoryProvider(Map.of())); | ||
| singleton.addHooks(new NoOpHook()); | ||
| singleton.setEvaluationContext(new ImmutableContext("singleton-key")); | ||
| singleton.setTransactionContextPropagator(new ThreadLocalTransactionContextPropagator()); | ||
|
|
||
| // isolated instance remains at baseline | ||
| assertThat(isolated.getProvider()).isSameAs(isolatedProvider); | ||
| assertThat(isolated.getHooks()).isEmpty(); | ||
| assertThat(isolated.getEvaluationContext()).isNull(); | ||
| assertThat(isolated.getTransactionContextPropagator()).isInstanceOf(NoOpTransactionContextPropagator.class); | ||
| } | ||
| } | ||
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.attachmentis not guaranteed to benullhere any more. If we want to ensure that, we need anAtomicReferenceand a cas operationThere 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.
+1 to this; we flagged the same race in our earlier review pass.
volatilegives visibility but not atomicity. AnAtomicReferencewithcompareAndSetwould be better I think, or just guardingattach()with a synchronized block since it's not on a hot path.