Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/main/java/dev/openfeature/sdk/EventProvider.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package dev.openfeature.sdk;

import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
import dev.openfeature.sdk.internal.ConfigurableThreadFactory;
import dev.openfeature.sdk.internal.TriConsumer;
import java.util.concurrent.ExecutorService;
Expand Down Expand Up @@ -30,20 +31,25 @@ void setEventProviderListener(EventProviderListener eventProviderListener) {
}

private TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit = null;
private AutoCloseableReentrantReadWriteLock lock = null;

/**
* "Attach" this EventProvider to an SDK, which allows events to propagate from this provider.
* No-op if the same onEmit is already attached.
*
* @param onEmit the function to run when a provider emits events.
* @param lock the API instance's read/write lock for thread safety.
* @throws IllegalStateException if attempted to bind a new emitter for already bound provider
*/
void attach(TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit) {
void attach(
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit,
AutoCloseableReentrantReadWriteLock lock) {
if (this.onEmit != null && this.onEmit != onEmit) {
// if we are trying to attach this provider to a different onEmit, something has gone wrong
throw new IllegalStateException("Provider " + this.getMetadata().getName() + " is already attached.");
} else {
this.onEmit = onEmit;
this.lock = lock;
}
}

Expand All @@ -52,6 +58,7 @@ void attach(TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEm
*/
void detach() {
this.onEmit = null;
this.lock = null;
}

/**
Expand Down Expand Up @@ -81,6 +88,7 @@ public void shutdown() {
public Awaitable emit(final ProviderEvent event, final ProviderEventDetails details) {
final var localEventProviderListener = this.eventProviderListener;
final var localOnEmit = this.onEmit;
final var localLock = this.lock;

if (localEventProviderListener == null && localOnEmit == null) {
return Awaitable.FINISHED;
Expand All @@ -91,7 +99,7 @@ public Awaitable emit(final ProviderEvent event, final ProviderEventDetails deta
// These calls need to be executed on a different thread to prevent deadlocks when the provider initialization
// relies on a ready event to be emitted
emitterExecutor.submit(() -> {
try (var ignored = OpenFeatureAPI.lock.readLockAutoCloseable()) {
try (var ignored = localLock != null ? localLock.readLockAutoCloseable() : null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the lock optional? IIRC, we needed that lock to avoid race conditions when emitting events

if (localEventProviderListener != null) {
localEventProviderListener.onEmit(event, details);
}
Expand Down
24 changes: 21 additions & 3 deletions src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
@Slf4j
@SuppressWarnings("PMD.UnusedLocalVariable")
public class OpenFeatureAPI implements EventBus<OpenFeatureAPI> {
// package-private multi-read/single-write lock
static AutoCloseableReentrantReadWriteLock lock = new AutoCloseableReentrantReadWriteLock();
// package-private multi-read/single-write lock (instance-level for isolation)
AutoCloseableReentrantReadWriteLock lock = new AutoCloseableReentrantReadWriteLock();
Comment thread
marcozabel marked this conversation as resolved.
Outdated
private final ConcurrentLinkedQueue<Hook> apiHooks;
private ProviderRepository providerRepository;
private EventSupport eventSupport;
Expand All @@ -50,6 +50,24 @@ public static OpenFeatureAPI getInstance() {
return SingletonHolder.INSTANCE;
}

/**
* Creates a new, independent {@link OpenFeatureAPI} instance with fully
* isolated state.
*
* <p>Each instance maintains its own providers, evaluation context, hooks,
* event handlers, and transaction context propagators. Instances do not
* share state with the global singleton or with each other.
*
* <p>For better discoverability, prefer using
* {@link dev.openfeature.sdk.isolated.OpenFeatureAPIFactory#createAPI()}.
*
* @return a new API instance
* @see dev.openfeature.sdk.isolated.OpenFeatureAPIFactory#createAPI()
*/
public static OpenFeatureAPI createIsolated() {
return new OpenFeatureAPI();
}

/**
* Get metadata about the default provider.
*
Expand Down Expand Up @@ -251,7 +269,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);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
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 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 &mdash; factory function for isolated instances</li>
* <li>Requirement 1.8.3 &mdash; distinct package for discoverability</li>
* </ul>
*
* @see <a href="https://openfeature.dev/specification/sections/flag-evaluation#18-isolated-api-instances">
* Spec &sect;1.8 &mdash; Isolated API Instances</a>
*/
public final class OpenFeatureAPIFactory {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 to flag that, e.g.:

* @apiNote This API is experimental and subject to change.

Could go on the class-level Javadoc or on createAPI() itself (or both).


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
* @see OpenFeatureAPI#createIsolated()
*/
public static OpenFeatureAPI createAPI() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully see the point of this method/class. OpenFeatureAPI.createIsolated() has the exact same visibility level and does the same thing. I think this will only lead to confusion

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

return OpenFeatureAPI.createIsolated();
}
}
20 changes: 13 additions & 7 deletions src/test/java/dev/openfeature/sdk/EventProviderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.*;

import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
import dev.openfeature.sdk.internal.TriConsumer;
import dev.openfeature.sdk.testutils.TestStackedEmitCallsProvider;
import io.cucumber.java.AfterAll;
Expand Down Expand Up @@ -36,7 +37,7 @@
@DisplayName("should run attached onEmit with emitters")
void emitsEventsWhenAttached() {
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit = mockOnEmit();
eventProvider.attach(onEmit);
eventProvider.attach(onEmit, new AutoCloseableReentrantReadWriteLock());

ProviderEventDetails details = ProviderEventDetails.builder().build();
eventProvider.emit(ProviderEvent.PROVIDER_READY, details);
Expand Down Expand Up @@ -73,17 +74,20 @@
void throwsWhenOnEmitDifferent() {
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit1 = mockOnEmit();
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit2 = mockOnEmit();
eventProvider.attach(onEmit1);
assertThrows(IllegalStateException.class, () -> eventProvider.attach(onEmit2));
eventProvider.attach(onEmit1, new AutoCloseableReentrantReadWriteLock());
assertThrows(

Check warning on line 78 in src/test/java/dev/openfeature/sdk/EventProviderTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor the code of the lambda to have only one invocation possibly throwing a runtime exception.

See more on https://sonarcloud.io/project/issues?id=open-feature_java-sdk&issues=AZ2W3U62d9hrC3kn3bYm&open=AZ2W3U62d9hrC3kn3bYm&pullRequest=1928
IllegalStateException.class,
() -> eventProvider.attach(onEmit2, new AutoCloseableReentrantReadWriteLock()));
}

@Test
@DisplayName("should not throw if second same onEmit attached")
void doesNotThrowWhenOnEmitSame() {
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit1 = mockOnEmit();
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit2 = onEmit1;
eventProvider.attach(onEmit1);
eventProvider.attach(onEmit2); // should not throw, same instance. noop
eventProvider.attach(onEmit1, new AutoCloseableReentrantReadWriteLock());
eventProvider.attach(
onEmit2, new AutoCloseableReentrantReadWriteLock()); // should not throw, same instance. noop
}

@Test
Expand Down Expand Up @@ -132,8 +136,10 @@
}

@Override
public void attach(TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit) {
super.attach(onEmit);
public void attach(
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit,
AutoCloseableReentrantReadWriteLock lock) {
super.attach(onEmit, lock);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ void beforeEach() {
client = (OpenFeatureClient) api.getClient("LockingTest");

apiLock = setupLock(apiLock, mockInnerReadLock(), mockInnerWriteLock());
OpenFeatureAPI.lock = apiLock;
api.lock = apiLock;
Comment thread
marcozabel marked this conversation as resolved.
Outdated

clientHooksLock = setupLock(clientHooksLock, mockInnerReadLock(), mockInnerWriteLock());
}
Expand Down
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() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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();
}
}
Loading
Loading