Skip to content

fix: Reduce locking and concurrency issues #1478

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.

Sign up for GitHub

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

Merged
toddbaert merged 8 commits into from
Jul 4, 2025
Merged

fix: Reduce locking and concurrency issues #1478

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a9d8daa
Reduce locking and concurrency issues
chrfwow Jun 12, 2025
fe7ed8b
Reduce locking and concurrency issues
chrfwow Jun 16, 2025
37e89f7
formatting
chrfwow Jun 16, 2025
b7816ae
use concurrent data structure for hooks
chrfwow Jun 18, 2025
ce92510
use concurrent data structure for hooks
chrfwow Jun 18, 2025
0eb7a1e
Merge branch 'main' into fix-sync-issues
toddbaert Jun 20, 2025
c7bd342
Merge branch 'main' into fix-sync-issues
chrfwow Jun 30, 2025
0e40497
Merge branch 'main' into fix-sync-issues
toddbaert Jul 4, 2025
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
68 changes: 33 additions & 35 deletions src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -24,7 +26,7 @@ public class OpenFeatureAPI implements EventBus<OpenFeatureAPI> {
private final List<Hook> apiHooks;

@chrfwow chrfwow Jun 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This ArrayList could also be replaced with some concurrent data structure, further reducing the usage of locks and object churn, but it might be slower in some cases

private ProviderRepository providerRepository;
private EventSupport eventSupport;
private EvaluationContext evaluationContext;
private final AtomicReference<EvaluationContext> evaluationContext = new AtomicReference<>();
private TransactionContextPropagator transactionContextPropagator;

protected OpenFeatureAPI() {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}
}
Expand All @@ -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;
}
}
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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()) {
Comment thread
toddbaert marked this conversation as resolved.
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
providerRepository.setProvider(
provider,
this::attachEventProvider,
Expand All @@ -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,
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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));
}
}
Expand All @@ -318,16 +314,20 @@ public void addHooks(Hook... hooks) {
* @return A list of {@link Hook}s.
*/
public List<Hook> 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);
}
}
}

/**
* Removes all hooks.
*/
public void clearHooks() {
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
this.apiHooks.clear();
}
}
Expand All @@ -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();

Expand Down Expand Up @@ -385,7 +385,7 @@ public OpenFeatureAPI onProviderError(Consumer<EventDetails> handler) {
*/
@Override
public OpenFeatureAPI on(ProviderEvent event, Consumer<EventDetails> handler) {
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
this.eventSupport.addGlobalHandler(event, handler);
return this;
}
Expand All @@ -396,18 +396,20 @@ public OpenFeatureAPI on(ProviderEvent event, Consumer<EventDetails> handler) {
*/
@Override
public OpenFeatureAPI removeHandler(ProviderEvent event, Consumer<EventDetails> handler) {
this.eventSupport.removeGlobalHandler(event, handler);
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {

@chrfwow chrfwow Jun 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It might be possible to remove locking on the eventSupport entirely, now that it uses concurrent collections.

@chrfwow chrfwow Jun 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Upon further review, I'm not so sure. We might get into trouble in the runHandlersForProvider, but it might also work just fine.
The problem lies rather with setting/getting the provider and its state from the providerRepository than anything we do in here.

@toddbaert toddbaert Jun 19, 2025

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.

You mean it's breaking in that the behavior changes, right (different references)? There's no associated compilation breakages here (or I'm missing something?).

If so, I don't think I would really qualify that as breaking... it's a bit of a grey area, but IMO if we don't make specific guarantees about references, we don't necessarily need to adhere to them. I think if we considered all behavior as completely unchangeable we couldn't even improve the locking 😅 .

@chrfwow chrfwow Jun 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I meant

this.eventSupport.removeGlobalHandler(event, handler);
}
return this;
}

void removeHandler(String domain, ProviderEvent event, Consumer<EventDetails> handler) {
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
eventSupport.removeClientHandler(domain, event, handler);
}
}

void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> 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)
Expand All @@ -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<String> 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<String> allDomainNames = eventSupport.getAllDomainNames();
Set<String> 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)));
}
}
}
Expand Down
30 changes: 16 additions & 14 deletions src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -49,8 +50,7 @@ public class OpenFeatureClient implements Client {
private final List<Hook> clientHooks;

@chrfwow chrfwow Jun 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This ArrayList could also be replaced with some concurrent data structure, further reducing the usage of locks

@toddbaert toddbaert Jun 17, 2025

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.

Ya it's final unlike the evaluationContext is, so we wouldn't need to even use an atomic reference.

Do you see any reason not to?

@chrfwow chrfwow Jun 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

private final HookSupport hookSupport;
AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock();
AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock();
private EvaluationContext evaluationContext;
private final AtomicReference<EvaluationContext> evaluationContext = new AtomicReference<>();

/**
* Deprecated public constructor. Use OpenFeature.API.getClient() instead.
Expand Down Expand Up @@ -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;
Expand All @@ -136,8 +136,12 @@ public OpenFeatureClient addHooks(Hook... hooks) {
*/
@Override
public List<Hook> 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);

@toddbaert toddbaert Jun 17, 2025

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 will create some additional object churn per evaluation, I guess you're worried about exposing this mutable state?

@chrfwow chrfwow Jun 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't want to expose a reference to the list, as this would effectively remove the lock entirely. At least the Collections.emptyList(); should not allocate anything.
I don't think it would add churn per evaluation, as I don't use the getter during the evaluation, but use the field directly.
Also consider that this and the corresponding change in OpenFeatureAPI is technically a breaking change

}
}
}

Expand All @@ -146,9 +150,7 @@ public List<Hook> getHooks() {
*/
@Override
public OpenFeatureClient setEvaluationContext(EvaluationContext evaluationContext) {
try (AutoCloseableLock __ = contextLock.writeLockAutoCloseable()) {
this.evaluationContext = evaluationContext;
}
this.evaluationContext.set(evaluationContext);
return this;
}

Expand All @@ -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 <T> FlagEvaluationDetails<T> evaluateFlag(
Expand All @@ -179,8 +179,10 @@ private <T> FlagEvaluationDetails<T> 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,
Expand Down Expand Up @@ -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);
}
Expand Down
25 changes: 0 additions & 25 deletions src/test/java/dev/openfeature/sdk/LockingSingeltonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class LockingSingeltonTest {
private static OpenFeatureAPI api;
private OpenFeatureClient client;
private AutoCloseableReentrantReadWriteLock apiLock;
private AutoCloseableReentrantReadWriteLock clientContextLock;
private AutoCloseableReentrantReadWriteLock clientHooksLock;

@BeforeAll
Expand All @@ -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;
}

Expand Down Expand Up @@ -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());
Expand Down
Loading