Skip to content

Commit ebea0fd

Browse files
chrfwowtoddbaert
chrfwow
and
toddbaert
authored
fix: Reduce locking and concurrency issues (#1478)
* Reduce locking and concurrency issues Signed-off-by: christian.lutnik <[email protected]> * Reduce locking and concurrency issues Signed-off-by: christian.lutnik <[email protected]> * formatting Signed-off-by: christian.lutnik <[email protected]> * use concurrent data structure for hooks Signed-off-by: christian.lutnik <[email protected]> * use concurrent data structure for hooks Signed-off-by: christian.lutnik <[email protected]> --------- Signed-off-by: christian.lutnik <[email protected]> Co-authored-by: Todd Baert <[email protected]>
1 parent 08f549a commit ebea0fd

File tree

4 files changed

+67
-129
lines changed

4 files changed

+67
-129
lines changed

‎src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java‎

Lines changed: 40 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@
55
import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
66
import java.util.ArrayList;
77
import java.util.Arrays;
8+
import java.util.Collection;
89
import java.util.List;
910
import java.util.Optional;
1011
import java.util.Set;
12+
import java.util.concurrent.ConcurrentLinkedQueue;
13+
import java.util.concurrent.atomic.AtomicReference;
1114
import java.util.function.Consumer;
1215
import lombok.extern.slf4j.Slf4j;
1316

@@ -21,14 +24,14 @@
2124
public class OpenFeatureAPI implements EventBus<OpenFeatureAPI> {
2225
// package-private multi-read/single-write lock
2326
static AutoCloseableReentrantReadWriteLock lock = new AutoCloseableReentrantReadWriteLock();
24-
private final List<Hook> apiHooks;
27+
private final ConcurrentLinkedQueue<Hook> apiHooks;
2528
private ProviderRepository providerRepository;
2629
private EventSupport eventSupport;
27-
private EvaluationContext evaluationContext;
30+
private final AtomicReference<EvaluationContext> evaluationContext = new AtomicReference<>();
2831
private TransactionContextPropagator transactionContextPropagator;
2932

3033
protected OpenFeatureAPI() {
31-
apiHooks = new ArrayList<>();
34+
apiHooks = new ConcurrentLinkedQueue<>();
3235
providerRepository = new ProviderRepository(this);
3336
eventSupport = new EventSupport();
3437
transactionContextPropagator = new NoOpTransactionContextPropagator();
@@ -115,9 +118,7 @@ public Client getClient(String domain, String version) {
115118
* @return api instance
116119
*/
117120
public OpenFeatureAPI setEvaluationContext(EvaluationContext evaluationContext) {
118-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
119-
this.evaluationContext = evaluationContext;
120-
}
121+
this.evaluationContext.set(evaluationContext);
121122
return this;
122123
}
123124

@@ -127,16 +128,14 @@ public OpenFeatureAPI setEvaluationContext(EvaluationContext evaluationContext)
127128
* @return evaluation context
128129
*/
129130
public EvaluationContext getEvaluationContext() {
130-
try (AutoCloseableLock __ = lock.readLockAutoCloseable()) {
131-
return this.evaluationContext;
132-
}
131+
return evaluationContext.get();
133132
}
134133

135134
/**
136135
* Return the transaction context propagator.
137136
*/
138137
public TransactionContextPropagator getTransactionContextPropagator() {
139-
try (AutoCloseableLock __ = lock.readLockAutoCloseable()) {
138+
try (AutoCloseableLock ignored = lock.readLockAutoCloseable()) {
140139
return this.transactionContextPropagator;
141140
}
142141
}
@@ -150,7 +149,7 @@ public void setTransactionContextPropagator(TransactionContextPropagator transac
150149
if (transactionContextPropagator == null) {
151150
throw new IllegalArgumentException("Transaction context propagator cannot be null");
152151
}
153-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
152+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
154153
this.transactionContextPropagator = transactionContextPropagator;
155154
}
156155
}
@@ -176,7 +175,7 @@ public void setTransactionContext(EvaluationContext evaluationContext) {
176175
* Set the default provider.
177176
*/
178177
public void setProvider(FeatureProvider provider) {
179-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
178+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
180179
providerRepository.setProvider(
181180
provider,
182181
this::attachEventProvider,
@@ -194,7 +193,7 @@ public void setProvider(FeatureProvider provider) {
194193
* @param provider The provider to set.
195194
*/
196195
public void setProvider(String domain, FeatureProvider provider) {
197-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
196+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
198197
providerRepository.setProvider(
199198
domain,
200199
provider,
@@ -216,7 +215,7 @@ public void setProvider(String domain, FeatureProvider provider) {
216215
* @throws OpenFeatureError if the provider fails during initialization.
217216
*/
218217
public void setProviderAndWait(FeatureProvider provider) throws OpenFeatureError {
219-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
218+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
220219
providerRepository.setProvider(
221220
provider,
222221
this::attachEventProvider,
@@ -238,7 +237,7 @@ public void setProviderAndWait(FeatureProvider provider) throws OpenFeatureError
238237
* @throws OpenFeatureError if the provider fails during initialization.
239238
*/
240239
public void setProviderAndWait(String domain, FeatureProvider provider) throws OpenFeatureError {
241-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
240+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
242241
providerRepository.setProvider(
243242
domain,
244243
provider,
@@ -252,9 +251,7 @@ public void setProviderAndWait(String domain, FeatureProvider provider) throws O
252251

253252
private void attachEventProvider(FeatureProvider provider) {
254253
if (provider instanceof EventProvider) {
255-
((EventProvider) provider).attach((p, event, details) -> {
256-
runHandlersForProvider(p, event, details);
257-
});
254+
((EventProvider) provider).attach(this::runHandlersForProvider);
258255
}
259256
}
260257

@@ -307,9 +304,7 @@ public FeatureProvider getProvider(String domain) {
307304
* @param hooks The hook to add.
308305
*/
309306
public void addHooks(Hook... hooks) {
310-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
311-
this.apiHooks.addAll(Arrays.asList(hooks));
312-
}
307+
this.apiHooks.addAll(Arrays.asList(hooks));
313308
}
314309

315310
/**
@@ -318,18 +313,23 @@ public void addHooks(Hook... hooks) {
318313
* @return A list of {@link Hook}s.
319314
*/
320315
public List<Hook> getHooks() {
321-
try (AutoCloseableLock __ = lock.readLockAutoCloseable()) {
322-
return this.apiHooks;
323-
}
316+
return new ArrayList<>(this.apiHooks);
317+
}
318+
319+
/**
320+
* Returns a reference to the collection of {@link Hook}s.
321+
*
322+
* @return The collection of {@link Hook}s.
323+
*/
324+
Collection<Hook> getMutableHooks() {
325+
return this.apiHooks;
324326
}
325327

326328
/**
327329
* Removes all hooks.
328330
*/
329331
public void clearHooks() {
330-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
331-
this.apiHooks.clear();
332-
}
332+
this.apiHooks.clear();
333333
}
334334

335335
/**
@@ -339,7 +339,7 @@ public void clearHooks() {
339339
* Once shut down is complete, API is reset and ready to use again.
340340
*/
341341
public void shutdown() {
342-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
342+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
343343
providerRepository.shutdown();
344344
eventSupport.shutdown();
345345

@@ -385,7 +385,7 @@ public OpenFeatureAPI onProviderError(Consumer<EventDetails> handler) {
385385
*/
386386
@Override
387387
public OpenFeatureAPI on(ProviderEvent event, Consumer<EventDetails> handler) {
388-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
388+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
389389
this.eventSupport.addGlobalHandler(event, handler);
390390
return this;
391391
}
@@ -396,18 +396,20 @@ public OpenFeatureAPI on(ProviderEvent event, Consumer<EventDetails> handler) {
396396
*/
397397
@Override
398398
public OpenFeatureAPI removeHandler(ProviderEvent event, Consumer<EventDetails> handler) {
399-
this.eventSupport.removeGlobalHandler(event, handler);
399+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
400+
this.eventSupport.removeGlobalHandler(event, handler);
401+
}
400402
return this;
401403
}
402404

403405
void removeHandler(String domain, ProviderEvent event, Consumer<EventDetails> handler) {
404-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
406+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
405407
eventSupport.removeClientHandler(domain, event, handler);
406408
}
407409
}
408410

409411
void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> handler) {
410-
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
412+
try (AutoCloseableLock ignored = lock.writeLockAutoCloseable()) {
411413
// if the provider is in the state associated with event, run immediately
412414
if (Optional.ofNullable(this.providerRepository.getProviderState(domain))
413415
.orElse(ProviderState.READY)
@@ -431,32 +433,28 @@ FeatureProviderStateManager getFeatureProviderStateManager(String domain) {
431433
* @param details the event details
432434
*/
433435
private void runHandlersForProvider(FeatureProvider provider, ProviderEvent event, ProviderEventDetails details) {
434-
try (AutoCloseableLock __ = lock.readLockAutoCloseable()) {
436+
try (AutoCloseableLock ignored = lock.readLockAutoCloseable()) {
435437

436438
List<String> domainsForProvider = providerRepository.getDomainsForProvider(provider);
437439

438440
final String providerName = Optional.ofNullable(provider.getMetadata())
439-
.map(metadata -> metadata.getName())
441+
.map(Metadata::getName)
440442
.orElse(null);
441443

442444
// run the global handlers
443445
eventSupport.runGlobalHandlers(event, EventDetails.fromProviderEventDetails(details, providerName));
444446

445447
// run the handlers associated with domains for this provider
446-
domainsForProvider.forEach(domain -> {
447-
eventSupport.runClientHandlers(
448-
domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain));
449-
});
448+
domainsForProvider.forEach(domain -> eventSupport.runClientHandlers(
449+
domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain)));
450450

451451
if (providerRepository.isDefaultProvider(provider)) {
452452
// run handlers for clients that have no bound providers (since this is the default)
453453
Set<String> allDomainNames = eventSupport.getAllDomainNames();
454454
Set<String> boundDomains = providerRepository.getAllBoundDomains();
455455
allDomainNames.removeAll(boundDomains);
456-
allDomainNames.forEach(domain -> {
457-
eventSupport.runClientHandlers(
458-
domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain));
459-
});
456+
allDomainNames.forEach(domain -> eventSupport.runClientHandlers(
457+
domain, event, EventDetails.fromProviderEventDetails(details, providerName, domain)));
460458
}
461459
}
462460
}

‎src/main/java/dev/openfeature/sdk/OpenFeatureClient.java‎

Lines changed: 24 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,17 @@
55
import dev.openfeature.sdk.exceptions.GeneralError;
66
import dev.openfeature.sdk.exceptions.OpenFeatureError;
77
import dev.openfeature.sdk.exceptions.ProviderNotReadyError;
8-
import dev.openfeature.sdk.internal.AutoCloseableLock;
9-
import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
108
import dev.openfeature.sdk.internal.ObjectUtils;
9+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
1110
import java.util.ArrayList;
1211
import java.util.Arrays;
1312
import java.util.Collections;
1413
import java.util.HashMap;
1514
import java.util.List;
1615
import java.util.Map;
1716
import java.util.Objects;
17+
import java.util.concurrent.ConcurrentLinkedQueue;
18+
import java.util.concurrent.atomic.AtomicReference;
1819
import java.util.function.Consumer;
1920
import lombok.Getter;
2021
import lombok.extern.slf4j.Slf4j;
@@ -46,11 +47,9 @@ public class OpenFeatureClient implements Client {
4647
@Getter
4748
private final String version;
4849

49-
private final List<Hook> clientHooks;
50+
private final ConcurrentLinkedQueue<Hook> clientHooks;
5051
private final HookSupport hookSupport;
51-
AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock();
52-
AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock();
53-
private EvaluationContext evaluationContext;
52+
private final AtomicReference<EvaluationContext> evaluationContext = new AtomicReference<>();
5453

5554
/**
5655
* Deprecated public constructor. Use OpenFeature.API.getClient() instead.
@@ -68,7 +67,7 @@ public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String domain, String ve
6867
this.openfeatureApi = openFeatureAPI;
6968
this.domain = domain;
7069
this.version = version;
71-
this.clientHooks = new ArrayList<>();
70+
this.clientHooks = new ConcurrentLinkedQueue<>();
7271
this.hookSupport = new HookSupport();
7372
}
7473

@@ -125,9 +124,7 @@ public void track(String trackingEventName, EvaluationContext context, TrackingE
125124
*/
126125
@Override
127126
public OpenFeatureClient addHooks(Hook... hooks) {
128-
try (AutoCloseableLock __ = this.hooksLock.writeLockAutoCloseable()) {
129-
this.clientHooks.addAll(Arrays.asList(hooks));
130-
}
127+
this.clientHooks.addAll(Arrays.asList(hooks));
131128
return this;
132129
}
133130

@@ -136,19 +133,15 @@ public OpenFeatureClient addHooks(Hook... hooks) {
136133
*/
137134
@Override
138135
public List<Hook> getHooks() {
139-
try (AutoCloseableLock __ = this.hooksLock.readLockAutoCloseable()) {
140-
return this.clientHooks;
141-
}
136+
return new ArrayList<>(this.clientHooks);
142137
}
143138

144139
/**
145140
* {@inheritDoc}
146141
*/
147142
@Override
148143
public OpenFeatureClient setEvaluationContext(EvaluationContext evaluationContext) {
149-
try (AutoCloseableLock __ = contextLock.writeLockAutoCloseable()) {
150-
this.evaluationContext = evaluationContext;
151-
}
144+
this.evaluationContext.set(evaluationContext);
152145
return this;
153146
}
154147

@@ -157,32 +150,33 @@ public OpenFeatureClient setEvaluationContext(EvaluationContext evaluationContex
157150
*/
158151
@Override
159152
public EvaluationContext getEvaluationContext() {
160-
try (AutoCloseableLock __ = contextLock.readLockAutoCloseable()) {
161-
return this.evaluationContext;
162-
}
153+
return this.evaluationContext.get();
163154
}
164155

156+
@SuppressFBWarnings(
157+
value = {"REC_CATCH_EXCEPTION"},
158+
justification = "We don't want to allow any exception to reach the user. "
159+
+ "Instead, we return an evaluation result with the appropriate error code.")
165160
private <T> FlagEvaluationDetails<T> evaluateFlag(
166161
FlagValueType type, String key, T defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
167-
FlagEvaluationOptions flagOptions = ObjectUtils.defaultIfNull(
162+
var flagOptions = ObjectUtils.defaultIfNull(
168163
options, () -> FlagEvaluationOptions.builder().build());
169-
Map<String, Object> hints = Collections.unmodifiableMap(flagOptions.getHookHints());
164+
var hints = Collections.unmodifiableMap(flagOptions.getHookHints());
170165

171166
FlagEvaluationDetails<T> details = null;
172167
List<Hook> mergedHooks = null;
173168
HookContext<T> afterHookContext = null;
174-
FeatureProvider provider;
175169

176170
try {
177-
FeatureProviderStateManager stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
171+
var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
178172
// provider must be accessed once to maintain a consistent reference
179-
provider = stateManager.getProvider();
180-
ProviderState state = stateManager.getState();
173+
var provider = stateManager.getProvider();
174+
var state = stateManager.getState();
181175

182176
mergedHooks = ObjectUtils.merge(
183-
provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks());
177+
provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks());
184178

185-
EvaluationContext mergedCtx = hookSupport.beforeHooks(
179+
var mergedCtx = hookSupport.beforeHooks(
186180
type,
187181
HookContext.from(
188182
key,
@@ -205,12 +199,12 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
205199
throw new FatalError("Provider is in an irrecoverable error state");
206200
}
207201

208-
ProviderEvaluation<T> providerEval =
202+
var providerEval =
209203
(ProviderEvaluation<T>) createProviderEvaluation(type, key, defaultValue, provider, mergedCtx);
210204

211205
details = FlagEvaluationDetails.from(providerEval, key);
212206
if (details.getErrorCode() != null) {
213-
OpenFeatureError error =
207+
var error =
214208
ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage());
215209
enrichDetailsWithErrorDefaults(defaultValue, details);
216210
hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints);
@@ -264,7 +258,7 @@ private void invokeTrack(String trackingEventName, EvaluationContext context, Tr
264258
*/
265259
private EvaluationContext mergeEvaluationContext(EvaluationContext invocationContext) {
266260
final EvaluationContext apiContext = openfeatureApi.getEvaluationContext();
267-
final EvaluationContext clientContext = this.getEvaluationContext();
261+
final EvaluationContext clientContext = evaluationContext.get();
268262
final EvaluationContext transactionContext = openfeatureApi.getTransactionContext();
269263
return mergeContextMaps(apiContext, transactionContext, clientContext, invocationContext);
270264
}

0 commit comments

Comments
 (0)