fix: Reduce locking and concurrency issues #1478

chrfwow · 2025-06-12T12:17:19Z

This PR

Reduces locking and potential concurrency issues

Signed-off-by: christian.lutnik <[email protected]>

chrfwow · 2025-06-12T12:18:22Z

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

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


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

chrfwow · 2025-06-12T12:19:18Z

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

@@ -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()) {


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

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.

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 😅 .

Yes, that's what I meant

chrfwow · 2025-06-12T12:19:40Z

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

@@ -49,8 +50,7 @@ public class OpenFeatureClient implements Client {
    private final List<Hook> clientHooks;


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

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?

Signed-off-by: christian.lutnik <[email protected]>

codecov · 2025-06-16T11:55:21Z

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.24%. Comparing base (08f549a) to head (0e40497).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...in/java/dev/openfeature/sdk/OpenFeatureClient.java	93.75%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1478      +/-   ##
============================================
+ Coverage     92.97%   93.24%   +0.26%     
  Complexity      488      488              
============================================
  Files            46       46              
  Lines          1182     1169      -13     
  Branches        103      103              
============================================
- Hits           1099     1090       -9     
+ Misses           53       49       -4     
  Partials         30       30

Flag	Coverage Δ
unittests	`93.24% <97.72%> (+0.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

toddbaert · 2025-06-17T13:24:38Z

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

+            if (this.clientHooks.isEmpty()) {
+                return Collections.emptyList();
+            } else {
+                return new ArrayList<>(this.clientHooks);


This will create some additional object churn per evaluation, I guess you're worried about exposing this mutable state?

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

toddbaert

Approved but consider https://github.com/open-feature/java-sdk/pull/1478/files#r2152272420

Signed-off-by: christian.lutnik <[email protected]>

chrfwow · 2025-06-18T08:43:43Z

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

+     *
+     * @return The collection of {@link Hook}s.
+     */
+    Collection<Hook> getMutableHooks() {


I really don't like this method, but we need some Method where we can return a ConcurrentLinkedQueue and don't have to convert it into a list to reduce memory churn during evaluations

chrfwow · 2025-06-18T08:47:21Z

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

-        try (AutoCloseableLock __ = this.hooksLock.readLockAutoCloseable()) {
-            return this.clientHooks;
-        }
+        return new ArrayList<>(this.clientHooks);


We can't return the ConcurrentLinkedQueue, as this method returns a List.

chrfwow · 2025-06-18T08:48:00Z

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


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


Directly access the openfeatureApi's ConcurrentLinkedQueue

Signed-off-by: christian.lutnik <[email protected]>

toddbaert · 2025-07-03T19:14:01Z

I'll merge this tomorrow unless I hear objections.

sonarqubecloud · 2025-07-04T13:00:53Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* 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]> Signed-off-by: suvaidkhan <[email protected]>

Reduce locking and concurrency issues

a9d8daa

Signed-off-by: christian.lutnik <[email protected]>

chrfwow requested review from a team as code owners June 12, 2025 12:17

chrfwow commented Jun 12, 2025

View reviewed changes

chrfwow changed the title ~~Reduce locking and concurrency issues~~ fix: Reduce locking and concurrency issues Jun 12, 2025

chrfwow added 2 commits June 16, 2025 13:43

Reduce locking and concurrency issues

fe7ed8b

Signed-off-by: christian.lutnik <[email protected]>

formatting

37e89f7

Signed-off-by: christian.lutnik <[email protected]>

chrfwow self-assigned this Jun 16, 2025

toddbaert reviewed Jun 17, 2025

View reviewed changes

src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java Show resolved Hide resolved

toddbaert reviewed Jun 17, 2025

View reviewed changes

toddbaert approved these changes Jun 17, 2025

View reviewed changes

use concurrent data structure for hooks

b7816ae

Signed-off-by: christian.lutnik <[email protected]>

chrfwow commented Jun 18, 2025

View reviewed changes

use concurrent data structure for hooks

ce92510

Signed-off-by: christian.lutnik <[email protected]>

chrfwow requested a review from toddbaert June 18, 2025 08:51

toddbaert requested review from justinabrahms, aepfli, liran2000, alexandraoberaigner and DBlanchard88 June 19, 2025 19:05

toddbaert approved these changes Jun 20, 2025

View reviewed changes

toddbaert and others added 2 commits June 20, 2025 10:58

Merge branch 'main' into fix-sync-issues

0eb7a1e

Merge branch 'main' into fix-sync-issues

c7bd342

Merge branch 'main' into fix-sync-issues

0e40497

toddbaert merged commit ebea0fd into open-feature:main Jul 4, 2025
13 checks passed

openfeaturebot mentioned this pull request Jul 4, 2025

chore(main): release 1.16.0 #1452

Merged

chrfwow deleted the fix-sync-issues branch July 7, 2025 09:49

toddbaert mentioned this pull request Jul 9, 2025

Provider compatibility issues after latest SDK release open-feature/java-sdk-contrib#1484

Closed

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

		@@ -49,8 +50,7 @@ public class OpenFeatureClient implements Client {
		private final List<Hook> clientHooks;

fix: Reduce locking and concurrency issues #1478

fix: Reduce locking and concurrency issues #1478

Uh oh!

Conversation

chrfwow commented Jun 12, 2025

This PR

Uh oh!

chrfwow Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrfwow Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrfwow Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddbaert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddbaert commented Jul 3, 2025

Uh oh!

sonarqubecloud bot commented Jul 4, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

chrfwow Jun 12, 2025 •

edited

Loading

chrfwow Jun 12, 2025 •

edited

Loading

codecov bot commented Jun 16, 2025 •

edited

Loading

chrfwow Jun 18, 2025 •

edited

Loading