-
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 8 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,6 +9,7 @@ | |
| 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; | ||
|
|
@@ -22,15 +23,30 @@ | |
| @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() { | ||
| 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 +267,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 +348,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,52 @@ | ||
| package dev.openfeature.sdk.isolated; | ||
|
|
||
| import dev.openfeature.sdk.OpenFeatureAPI; | ||
|
|
||
| /** | ||
| * Factory for creating isolated OpenFeature API instances. | ||
| * | ||
| * <p>Each instance returned by {@link #createAPI()} maintains its own state, | ||
| * including providers, evaluation context, hooks, event handlers, and | ||
| * transaction context propagators. Instances do not share state with the | ||
| * global singleton ({@link OpenFeatureAPI#getInstance()}) or with each other. | ||
| * | ||
| * <p>This class lives in a distinct package ({@code dev.openfeature.sdk.isolated}) | ||
| * to make isolated instances intentionally less discoverable than the global | ||
| * singleton, reducing the chance of accidental use when the singleton would be | ||
| * appropriate. | ||
| * | ||
| * <p>This is useful for dependency injection frameworks, testing scenarios, | ||
| * and applications composed of multiple submodules requiring distinct providers. | ||
| * | ||
| * <p><strong>Spec references:</strong> | ||
| * <ul> | ||
| * <li>Requirement 1.8.1 — factory function for isolated instances</li> | ||
| * <li>Requirement 1.8.3 — factory in a distinct package/module</li> | ||
| * </ul> | ||
| * | ||
| * @see <a href="https://openfeature.dev/specification/sections/flag-evaluation#18-isolated-api-instances"> | ||
| * Spec §1.8 — Isolated API Instances</a> | ||
| */ | ||
| public final class OpenFeatureAPIFactory { | ||
|
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. Section 1.8 is marked experimental in the spec. Might be worth adding an * @apiNote This API is experimental and subject to change.Could go on the class-level Javadoc or on |
||
|
|
||
| private OpenFeatureAPIFactory() { | ||
| // utility class | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new, independent {@link OpenFeatureAPI} instance with fully | ||
| * isolated state. | ||
| * | ||
| * <p>Usage: | ||
| * <pre>{@code | ||
| * OpenFeatureAPI api = OpenFeatureAPIFactory.createAPI(); | ||
| * api.setProvider(new MyProvider()); | ||
| * Client client = api.getClient(); | ||
| * }</pre> | ||
| * | ||
| * @return a new API instance | ||
| */ | ||
| public static OpenFeatureAPI createAPI() { | ||
|
Contributor
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 don't fully see the point of this method/class.
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. Agree |
||
| return new OpenFeatureAPI(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| 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.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 = OpenFeatureAPIFactory.createAPI(); | ||
| 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(); | ||
| } | ||
| } | ||
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.