From a9d8daa54f33374d8abfdd5f0fbc26e757d52a29 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Thu, 12 Jun 2025 14:14:53 +0200 Subject: [PATCH 1/5] Reduce locking and concurrency issues Signed-off-by: christian.lutnik --- .../dev/openfeature/sdk/OpenFeatureAPI.java | 68 +++++++++---------- .../openfeature/sdk/OpenFeatureClient.java | 30 ++++---- .../openfeature/sdk/LockingSingeltonTest.java | 25 ------- 3 files changed, 49 insertions(+), 74 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index 66d40736f..6733c8bf5 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java @@ -5,9 +5,11 @@ import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import lombok.extern.slf4j.Slf4j; @@ -24,7 +26,7 @@ public class OpenFeatureAPI implements EventBus { private final List apiHooks; private ProviderRepository providerRepository; private EventSupport eventSupport; - private EvaluationContext evaluationContext; + private final AtomicReference evaluationContext = new AtomicReference<>(); private TransactionContextPropagator transactionContextPropagator; protected OpenFeatureAPI() { @@ -115,9 +117,7 @@ public Client getClient(String domain, String version) { * @return api instance */ public OpenFeatureAPI setEvaluationContext(EvaluationContext evaluationContext) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { - this.evaluationContext = evaluationContext; - } + this.evaluationContext.set(evaluationContext); return this; } @@ -127,16 +127,14 @@ public OpenFeatureAPI setEvaluationContext(EvaluationContext evaluationContext) * @return evaluation context */ public EvaluationContext getEvaluationContext() { - try (AutoCloseableLock __ = lock.readLockAutoCloseable()) { - return this.evaluationContext; - } + return evaluationContext.get(); } /** * Return the transaction context propagator. */ public TransactionContextPropagator getTransactionContextPropagator() { - try (AutoCloseableLock __ = lock.readLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.readLockAutoCloseable()) { return this.transactionContextPropagator; } } @@ -150,7 +148,7 @@ public void setTransactionContextPropagator(TransactionContextPropagator transac if (transactionContextPropagator == null) { throw new IllegalArgumentException("Transaction context propagator cannot be null"); } - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { this.transactionContextPropagator = transactionContextPropagator; } } @@ -176,7 +174,7 @@ public void setTransactionContext(EvaluationContext evaluationContext) { * Set the default provider. */ public void setProvider(FeatureProvider provider) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { providerRepository.setProvider( provider, this::attachEventProvider, @@ -194,7 +192,7 @@ public void setProvider(FeatureProvider provider) { * @param provider The provider to set. */ public void setProvider(String domain, FeatureProvider provider) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { providerRepository.setProvider( domain, provider, @@ -216,7 +214,7 @@ public void setProvider(String domain, FeatureProvider provider) { * @throws OpenFeatureError if the provider fails during initialization. */ public void setProviderAndWait(FeatureProvider provider) throws OpenFeatureError { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { providerRepository.setProvider( provider, this::attachEventProvider, @@ -238,7 +236,7 @@ public void setProviderAndWait(FeatureProvider provider) throws OpenFeatureError * @throws OpenFeatureError if the provider fails during initialization. */ public void setProviderAndWait(String domain, FeatureProvider provider) throws OpenFeatureError { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { providerRepository.setProvider( domain, provider, @@ -252,9 +250,7 @@ public void setProviderAndWait(String domain, FeatureProvider provider) throws O private void attachEventProvider(FeatureProvider provider) { if (provider instanceof EventProvider) { - ((EventProvider) provider).attach((p, event, details) -> { - runHandlersForProvider(p, event, details); - }); + ((EventProvider) provider).attach(this::runHandlersForProvider); } } @@ -307,7 +303,7 @@ public FeatureProvider getProvider(String domain) { * @param hooks The hook to add. */ public void addHooks(Hook... hooks) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { this.apiHooks.addAll(Arrays.asList(hooks)); } } @@ -318,8 +314,12 @@ public void addHooks(Hook... hooks) { * @return A list of {@link Hook}s. */ public List getHooks() { - try (AutoCloseableLock __ = lock.readLockAutoCloseable()) { - return this.apiHooks; + try (AutoCloseableLock ignored = lock.readLockAutoCloseable()) { + if (this.apiHooks.isEmpty()) { + return Collections.emptyList(); + } else { + return new ArrayList<>(this.apiHooks); + } } } @@ -327,7 +327,7 @@ public List getHooks() { * Removes all hooks. */ public void clearHooks() { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { this.apiHooks.clear(); } } @@ -339,7 +339,7 @@ public void clearHooks() { * Once shut down is complete, API is reset and ready to use again. */ public void shutdown() { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { providerRepository.shutdown(); eventSupport.shutdown(); @@ -385,7 +385,7 @@ public OpenFeatureAPI onProviderError(Consumer handler) { */ @Override public OpenFeatureAPI on(ProviderEvent event, Consumer handler) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { this.eventSupport.addGlobalHandler(event, handler); return this; } @@ -396,18 +396,20 @@ public OpenFeatureAPI on(ProviderEvent event, Consumer handler) { */ @Override public OpenFeatureAPI removeHandler(ProviderEvent event, Consumer handler) { - this.eventSupport.removeGlobalHandler(event, handler); + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { + this.eventSupport.removeGlobalHandler(event, handler); + } return this; } void removeHandler(String domain, ProviderEvent event, Consumer handler) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { eventSupport.removeClientHandler(domain, event, handler); } } void addHandler(String domain, ProviderEvent event, Consumer handler) { - try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { // if the provider is in the state associated with event, run immediately if (Optional.ofNullable(this.providerRepository.getProviderState(domain)) .orElse(ProviderState.READY) @@ -431,32 +433,28 @@ FeatureProviderStateManager getFeatureProviderStateManager(String domain) { * @param details the event details */ private void runHandlersForProvider(FeatureProvider provider, ProviderEvent event, ProviderEventDetails details) { - try (AutoCloseableLock __ = lock.readLockAutoCloseable()) { + try (AutoCloseableLock ignored = lock.readLockAutoCloseable()) { List domainsForProvider = providerRepository.getDomainsForProvider(provider); final String providerName = Optional.ofNullable(provider.getMetadata()) - .map(metadata -> metadata.getName()) + .map(Metadata::getName) .orElse(null); // run the global handlers eventSupport.runGlobalHandlers(event, EventDetails.fromProviderEventDetails(details, providerName)); // run the handlers associated with domains for this provider - domainsForProvider.forEach(domain -> { - eventSupport.runClientHandlers( - domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain)); - }); + domainsForProvider.forEach(domain -> eventSupport.runClientHandlers( + domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain))); if (providerRepository.isDefaultProvider(provider)) { // run handlers for clients that have no bound providers (since this is the default) Set allDomainNames = eventSupport.getAllDomainNames(); Set boundDomains = providerRepository.getAllBoundDomains(); allDomainNames.removeAll(boundDomains); - allDomainNames.forEach(domain -> { - eventSupport.runClientHandlers( - domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain)); - }); + allDomainNames.forEach(domain -> eventSupport.runClientHandlers( + domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain))); } } } diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index e68d28f79..7f1956fbe 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -49,8 +50,7 @@ public class OpenFeatureClient implements Client { private final List clientHooks; private final HookSupport hookSupport; AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock(); - AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock(); - private EvaluationContext evaluationContext; + private final AtomicReference evaluationContext = new AtomicReference<>(); /** * Deprecated public constructor. Use OpenFeature.API.getClient() instead. @@ -125,7 +125,7 @@ public void track(String trackingEventName, EvaluationContext context, TrackingE */ @Override public OpenFeatureClient addHooks(Hook... hooks) { - try (AutoCloseableLock __ = this.hooksLock.writeLockAutoCloseable()) { + try (AutoCloseableLock ignored = this.hooksLock.writeLockAutoCloseable()) { this.clientHooks.addAll(Arrays.asList(hooks)); } return this; @@ -136,8 +136,12 @@ public OpenFeatureClient addHooks(Hook... hooks) { */ @Override public List getHooks() { - try (AutoCloseableLock __ = this.hooksLock.readLockAutoCloseable()) { - return this.clientHooks; + try (AutoCloseableLock ignored = this.hooksLock.readLockAutoCloseable()) { + if (this.clientHooks.isEmpty()) { + return Collections.emptyList(); + } else { + return new ArrayList<>(this.clientHooks); + } } } @@ -146,9 +150,7 @@ public List getHooks() { */ @Override public OpenFeatureClient setEvaluationContext(EvaluationContext evaluationContext) { - try (AutoCloseableLock __ = contextLock.writeLockAutoCloseable()) { - this.evaluationContext = evaluationContext; - } + this.evaluationContext.set(evaluationContext); return this; } @@ -157,9 +159,7 @@ public OpenFeatureClient setEvaluationContext(EvaluationContext evaluationContex */ @Override public EvaluationContext getEvaluationContext() { - try (AutoCloseableLock __ = contextLock.readLockAutoCloseable()) { - return this.evaluationContext; - } + return this.evaluationContext.get(); } private FlagEvaluationDetails evaluateFlag( @@ -179,8 +179,10 @@ private FlagEvaluationDetails evaluateFlag( provider = stateManager.getProvider(); ProviderState state = stateManager.getState(); - mergedHooks = ObjectUtils.merge( - provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks()); + try (AutoCloseableLock ignored = this.hooksLock.readLockAutoCloseable()) { + mergedHooks = ObjectUtils.merge( + provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks()); + } EvaluationContext mergedCtx = hookSupport.beforeHooks( type, @@ -264,7 +266,7 @@ private void invokeTrack(String trackingEventName, EvaluationContext context, Tr */ private EvaluationContext mergeEvaluationContext(EvaluationContext invocationContext) { final EvaluationContext apiContext = openfeatureApi.getEvaluationContext(); - final EvaluationContext clientContext = this.getEvaluationContext(); + final EvaluationContext clientContext = evaluationContext.get(); final EvaluationContext transactionContext = openfeatureApi.getTransactionContext(); return mergeContextMaps(apiContext, transactionContext, clientContext, invocationContext); } diff --git a/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java b/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java index ad86f4bc3..913dcc68c 100644 --- a/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java +++ b/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java @@ -20,7 +20,6 @@ class LockingSingeltonTest { private static OpenFeatureAPI api; private OpenFeatureClient client; private AutoCloseableReentrantReadWriteLock apiLock; - private AutoCloseableReentrantReadWriteLock clientContextLock; private AutoCloseableReentrantReadWriteLock clientHooksLock; @BeforeAll @@ -36,9 +35,7 @@ void beforeEach() { apiLock = setupLock(apiLock, mockInnerReadLock(), mockInnerWriteLock()); OpenFeatureAPI.lock = apiLock; - clientContextLock = setupLock(clientContextLock, mockInnerReadLock(), mockInnerWriteLock()); clientHooksLock = setupLock(clientHooksLock, mockInnerReadLock(), mockInnerWriteLock()); - client.contextLock = clientContextLock; client.hooksLock = clientHooksLock; } @@ -159,28 +156,6 @@ void getHooksShouldReadLockAndUnlock() { verify(apiLock.readLock()).unlock(); } - @Test - void setContextShouldWriteLockAndUnlock() { - client.setEvaluationContext(new ImmutableContext()); - verify(clientContextLock.writeLock()).lock(); - verify(clientContextLock.writeLock()).unlock(); - - api.setEvaluationContext(new ImmutableContext()); - verify(apiLock.writeLock()).lock(); - verify(apiLock.writeLock()).unlock(); - } - - @Test - void getContextShouldReadLockAndUnlock() { - client.getEvaluationContext(); - verify(clientContextLock.readLock()).lock(); - verify(clientContextLock.readLock()).unlock(); - - api.getEvaluationContext(); - verify(apiLock.readLock()).lock(); - verify(apiLock.readLock()).unlock(); - } - @Test void setTransactionalContextPropagatorShouldWriteLockAndUnlock() { api.setTransactionContextPropagator(new NoOpTransactionContextPropagator()); From fe7ed8b5659c146f4f830dbd6cf8b2a37eb804e0 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Mon, 16 Jun 2025 13:43:15 +0200 Subject: [PATCH 2/5] Reduce locking and concurrency issues Signed-off-by: christian.lutnik --- .../openfeature/sdk/OpenFeatureClient.java | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 7f1956fbe..b5c25e3fe 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -8,6 +8,7 @@ import dev.openfeature.sdk.internal.AutoCloseableLock; import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; import dev.openfeature.sdk.internal.ObjectUtils; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -162,29 +163,32 @@ public EvaluationContext getEvaluationContext() { return this.evaluationContext.get(); } + @SuppressFBWarnings( + value = {"REC_CATCH_EXCEPTION"}, + justification = + "We don't want to allow any exception to reach the user. Instead, we return an evaluation result with the appropriate error code.") private FlagEvaluationDetails evaluateFlag( FlagValueType type, String key, T defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) { - FlagEvaluationOptions flagOptions = ObjectUtils.defaultIfNull( + var flagOptions = ObjectUtils.defaultIfNull( options, () -> FlagEvaluationOptions.builder().build()); - Map hints = Collections.unmodifiableMap(flagOptions.getHookHints()); + var hints = Collections.unmodifiableMap(flagOptions.getHookHints()); FlagEvaluationDetails details = null; List mergedHooks = null; HookContext afterHookContext = null; - FeatureProvider provider; try { - FeatureProviderStateManager stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); + var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); // provider must be accessed once to maintain a consistent reference - provider = stateManager.getProvider(); - ProviderState state = stateManager.getState(); + var provider = stateManager.getProvider(); + var state = stateManager.getState(); try (AutoCloseableLock ignored = this.hooksLock.readLockAutoCloseable()) { mergedHooks = ObjectUtils.merge( provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks()); } - EvaluationContext mergedCtx = hookSupport.beforeHooks( + var mergedCtx = hookSupport.beforeHooks( type, HookContext.from( key, @@ -207,12 +211,12 @@ private FlagEvaluationDetails evaluateFlag( throw new FatalError("Provider is in an irrecoverable error state"); } - ProviderEvaluation providerEval = + var providerEval = (ProviderEvaluation) createProviderEvaluation(type, key, defaultValue, provider, mergedCtx); details = FlagEvaluationDetails.from(providerEval, key); if (details.getErrorCode() != null) { - OpenFeatureError error = + var error = ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints); From 37e89f716f6b9cc7080df65486616b787297edbd Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Mon, 16 Jun 2025 13:50:27 +0200 Subject: [PATCH 3/5] formatting Signed-off-by: christian.lutnik --- src/main/java/dev/openfeature/sdk/OpenFeatureClient.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index b5c25e3fe..89d6a39b2 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -165,8 +165,8 @@ public EvaluationContext getEvaluationContext() { @SuppressFBWarnings( value = {"REC_CATCH_EXCEPTION"}, - justification = - "We don't want to allow any exception to reach the user. Instead, we return an evaluation result with the appropriate error code.") + justification = "We don't want to allow any exception to reach the user. " + + "Instead, we return an evaluation result with the appropriate error code.") private FlagEvaluationDetails evaluateFlag( FlagValueType type, String key, T defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) { var flagOptions = ObjectUtils.defaultIfNull( From b7816ae45c939d2104dd57e9c284aa3caa68c747 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Wed, 18 Jun 2025 10:42:35 +0200 Subject: [PATCH 4/5] use concurrent data structure for hooks Signed-off-by: christian.lutnik --- .../dev/openfeature/sdk/OpenFeatureAPI.java | 32 +++++++++---------- .../openfeature/sdk/OpenFeatureClient.java | 26 ++++----------- .../openfeature/sdk/internal/ObjectUtils.java | 5 +-- 3 files changed, 26 insertions(+), 37 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index 6733c8bf5..6d0d8feb4 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java @@ -5,10 +5,11 @@ import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import lombok.extern.slf4j.Slf4j; @@ -23,14 +24,14 @@ public class OpenFeatureAPI implements EventBus { // package-private multi-read/single-write lock static AutoCloseableReentrantReadWriteLock lock = new AutoCloseableReentrantReadWriteLock(); - private final List apiHooks; + private final ConcurrentLinkedQueue apiHooks; private ProviderRepository providerRepository; private EventSupport eventSupport; private final AtomicReference evaluationContext = new AtomicReference<>(); private TransactionContextPropagator transactionContextPropagator; protected OpenFeatureAPI() { - apiHooks = new ArrayList<>(); + apiHooks = new ConcurrentLinkedQueue<>(); providerRepository = new ProviderRepository(this); eventSupport = new EventSupport(); transactionContextPropagator = new NoOpTransactionContextPropagator(); @@ -303,9 +304,7 @@ public FeatureProvider getProvider(String domain) { * @param hooks The hook to add. */ public void addHooks(Hook... hooks) { - try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { - this.apiHooks.addAll(Arrays.asList(hooks)); - } + this.apiHooks.addAll(Arrays.asList(hooks)); } /** @@ -314,22 +313,23 @@ public void addHooks(Hook... hooks) { * @return A list of {@link Hook}s. */ public List getHooks() { - try (AutoCloseableLock ignored = lock.readLockAutoCloseable()) { - if (this.apiHooks.isEmpty()) { - return Collections.emptyList(); - } else { - return new ArrayList<>(this.apiHooks); - } - } + return new ArrayList<>(this.apiHooks); + } + + /** + * Returns a reference to the collection of {@link Hook}s. + * + * @return The collection of {@link Hook}s. + */ + Collection getMutableHooks() { + return this.apiHooks; } /** * Removes all hooks. */ public void clearHooks() { - try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) { - this.apiHooks.clear(); - } + this.apiHooks.clear(); } /** diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 89d6a39b2..b5522b66a 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -5,8 +5,6 @@ import dev.openfeature.sdk.exceptions.GeneralError; import dev.openfeature.sdk.exceptions.OpenFeatureError; import dev.openfeature.sdk.exceptions.ProviderNotReadyError; -import dev.openfeature.sdk.internal.AutoCloseableLock; -import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; import dev.openfeature.sdk.internal.ObjectUtils; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; @@ -16,6 +14,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import lombok.Getter; @@ -48,9 +47,8 @@ public class OpenFeatureClient implements Client { @Getter private final String version; - private final List clientHooks; + private final ConcurrentLinkedQueue clientHooks; private final HookSupport hookSupport; - AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock(); private final AtomicReference evaluationContext = new AtomicReference<>(); /** @@ -69,7 +67,7 @@ public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String domain, String ve this.openfeatureApi = openFeatureAPI; this.domain = domain; this.version = version; - this.clientHooks = new ArrayList<>(); + this.clientHooks = new ConcurrentLinkedQueue<>(); this.hookSupport = new HookSupport(); } @@ -126,9 +124,7 @@ public void track(String trackingEventName, EvaluationContext context, TrackingE */ @Override public OpenFeatureClient addHooks(Hook... hooks) { - try (AutoCloseableLock ignored = this.hooksLock.writeLockAutoCloseable()) { - this.clientHooks.addAll(Arrays.asList(hooks)); - } + this.clientHooks.addAll(Arrays.asList(hooks)); return this; } @@ -137,13 +133,7 @@ public OpenFeatureClient addHooks(Hook... hooks) { */ @Override public List getHooks() { - try (AutoCloseableLock ignored = this.hooksLock.readLockAutoCloseable()) { - if (this.clientHooks.isEmpty()) { - return Collections.emptyList(); - } else { - return new ArrayList<>(this.clientHooks); - } - } + return new ArrayList<>(this.clientHooks); } /** @@ -183,10 +173,8 @@ private FlagEvaluationDetails evaluateFlag( var provider = stateManager.getProvider(); var state = stateManager.getState(); - try (AutoCloseableLock ignored = this.hooksLock.readLockAutoCloseable()) { - mergedHooks = ObjectUtils.merge( - provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks()); - } + mergedHooks = ObjectUtils.merge( + provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks()); var mergedCtx = hookSupport.beforeHooks( type, diff --git a/src/main/java/dev/openfeature/sdk/internal/ObjectUtils.java b/src/main/java/dev/openfeature/sdk/internal/ObjectUtils.java index b367820c2..86a9ddd70 100644 --- a/src/main/java/dev/openfeature/sdk/internal/ObjectUtils.java +++ b/src/main/java/dev/openfeature/sdk/internal/ObjectUtils.java @@ -1,6 +1,7 @@ package dev.openfeature.sdk.internal; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.function.Supplier; @@ -64,9 +65,9 @@ public static T defaultIfNull(T source, Supplier defaultValue) { * @return resulting object */ @SafeVarargs - public static List merge(List... sources) { + public static List merge(Collection... sources) { List merged = new ArrayList<>(); - for (List source : sources) { + for (Collection source : sources) { merged.addAll(source); } return merged; From ce92510d6a7d6b054588cf65ed07addc4cef3d3a Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Wed, 18 Jun 2025 10:51:37 +0200 Subject: [PATCH 5/5] use concurrent data structure for hooks Signed-off-by: christian.lutnik --- .../openfeature/sdk/LockingSingeltonTest.java | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java b/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java index 913dcc68c..ae3246cae 100644 --- a/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java +++ b/src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java @@ -36,7 +36,6 @@ void beforeEach() { OpenFeatureAPI.lock = apiLock; clientHooksLock = setupLock(clientHooksLock, mockInnerReadLock(), mockInnerWriteLock()); - client.hooksLock = clientHooksLock; } @Nested @@ -134,28 +133,6 @@ void onProviderErrorProviderReadyShouldApiWriteLockAndUnlock() { } } - @Test - void addHooksShouldWriteLockAndUnlock() { - client.addHooks(new Hook() {}); - verify(clientHooksLock.writeLock()).lock(); - verify(clientHooksLock.writeLock()).unlock(); - - api.addHooks(new Hook() {}); - verify(apiLock.writeLock()).lock(); - verify(apiLock.writeLock()).unlock(); - } - - @Test - void getHooksShouldReadLockAndUnlock() { - client.getHooks(); - verify(clientHooksLock.readLock()).lock(); - verify(clientHooksLock.readLock()).unlock(); - - api.getHooks(); - verify(apiLock.readLock()).lock(); - verify(apiLock.readLock()).unlock(); - } - @Test void setTransactionalContextPropagatorShouldWriteLockAndUnlock() { api.setTransactionContextPropagator(new NoOpTransactionContextPropagator()); @@ -170,13 +147,6 @@ void getTransactionalContextPropagatorShouldReadLockAndUnlock() { verify(apiLock.readLock()).unlock(); } - @Test - void clearHooksShouldWriteLockAndUnlock() { - api.clearHooks(); - verify(apiLock.writeLock()).lock(); - verify(apiLock.writeLock()).unlock(); - } - private static ReentrantReadWriteLock.ReadLock mockInnerReadLock() { ReentrantReadWriteLock.ReadLock readLockMock = mock(ReentrantReadWriteLock.ReadLock.class); doNothing().when(readLockMock).lock();