fix: Use ConcurrentHashMap for InMemoryProvider #1057

ryanprayogo · 2024-08-21T02:15:40Z

Use ConcurrentHashMap for InMemoryProvider

Use ConcurrentHashMap for flags in InMemoryProvider to make it thread-safe

Related Issues

Fixes #1054

Notes

Follow-up Tasks

How to test

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

codecov · 2024-08-21T02:58:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.25%. Comparing base (a81957f) to head (b62f4ea).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1057      +/-   ##
============================================
+ Coverage     95.04%   95.25%   +0.21%     
- Complexity      392      393       +1     
============================================
  Files            38       38              
  Lines           888      886       -2     
  Branches         54       54              
============================================
  Hits            844      844              
+ Misses           24       23       -1     
+ Partials         20       19       -1

Flag	Coverage Δ
unittests	`95.25% <100.00%> (+0.21%)`	⬆️

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.

AndiHofi

This is still not thread-safe.

Either the flags field has to be final and modified in-place.

Or use a normal HashMap and a lock for each access to the map.

Or an AtomicReference and the feature flags are held in an immutable Map (does not need to be immutable, but it prevents from changing the map accidentally after setting it to the AtomicReference.

The simplest option is to make the flags field final and update it with a normal put(key, value)

Also, in updateFlags(Map flags) the flagsChanged set should only contain the keys of the changed flags, not all flags the provider. That bug existed earlier already.

The test is likely unstable, independent if the InMemoryProvider is threadsafe or not. There is no guarantee that any of the scheduled tasks even started execution when the assertion for that flag runs.

toddbaert · 2024-08-21T14:59:52Z

This is still not thread-safe.

Either the flags field has to be final and modified in-place.

Or use a normal HashMap and a lock for each access to the map.

Or an AtomicReference and the feature flags are held in an immutable Map (does not need to be immutable, but it prevents from changing the map accidentally after setting it to the AtomicReference.

The simplest option is to make the flags field final and update it with a normal put(key, value)

Also, in updateFlags(Map flags) the flagsChanged set should only contain the keys of the changed flags, not all flags the provider. That bug existed earlier already.

I believe this is all correct. The use of the concurrent map is safer but there are still thread safety issues with this new implementaiton

The test is likely unstable, independent if the InMemoryProvider is threadsafe or not. There is no guarantee that any of the scheduled tasks even started execution when the assertion for that flag runs.

Yes, I generally find this sort of thing difficult to test, which is why I don't think testing this is a hard requirement. As long as there's a consensus on the thread safety I think we're OK to merge.

toddbaert

An improvement, certainly! but still a bit to go: #1057 (comment)

🙏

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java

toddbaert · 2024-08-21T19:06:42Z

src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java

+    @Test
+    void multithreadedTest() throws InterruptedException {
+        ExecutorService executorService = Executors.newFixedThreadPool(100);
+        List<Callable<Void>> updates = new ArrayList<>();
+        for (int i = 0; i < 10000; i++) {
+            String flagKey = "multithreadedFlag" + i;
+            updates.add(() -> {
+                provider.updateFlag(flagKey, Flag.builder()
+                        .variant("on", true)
+                        .variant("off", false)
+                        .defaultVariant("on")
+                        .build());
+                return null;
+            });
+        }
+
+        executorService.invokeAll(updates);
+
+        for (int i = 0; i < 10000; i++) {
+            assertTrue(client.getBooleanValue("multithreadedFlag" + i, false));
+        }
+    }


I'm a bit dubious of the value of this test. Did you actually see it fail at least sometimes without your changes?

I did write this test before making any changes and did see it had failed sometimes.

After my change, I have not observed any failures in this test.

toddbaert

I think this covers it! Thank you!

I'll wait a bit for a follow-up from @AndiHofi since their observations were pretty astute before.

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

toddbaert · 2024-08-22T13:01:10Z

src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java

+
+        provider.updateFlags(flags);
+
+        verify(handler, times(1))


Oh, I think this test is a bit flaky. You probably will need to use Awaitility to make this assertion within some time frame, since handlers run asynchronously on another thread. If you search for await in this repo you'll see a bunch of similar examples.

I did this here since it was a quick change. I hope you don't mind @ryanprayogo

@toddbaert I do not mind, thank you.

Would it be worth adding an .atMost(...) so the test doesn't hang forever in the case that the assertion doesn't pass?

There's a default time limit (I think 5s?). After that point the test fails with a timeout.

Ah yes, you're right 👌

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

AndiHofi

See comments in code

AndiHofi · 2024-08-22T14:41:58Z

src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java

-        this.flags = new HashMap<>(flags);
+    public void updateFlags(Map<String, Flag<?>> newFlags) {
+        Set<String> flagsChanged = new HashSet<>(newFlags.keySet());
+        flagsChanged.removeAll(this.flags.keySet());


This is not needed.

flagsChanged has to be new HashSet(newFlags.keySet())
otherwise it would be "flagsAdded" ... that value is used to notify listeners that a flag value may have changed and that they need to read the feature flag value again.

Then it matches the behavior of updateFlag(String flagKey, Flag newFlag) that also sends the update event for the modified flag in all cases.

What would be the perfect solution is something like this: (pseudo-code)

Set flagsChanged = new HashSet(); // set of all flag keys that got modified, or added. for (Map.Entry changed : newFlags.entrySet()) { if (!Objects.equals(changed.getValue(), this.flags.get(changed.getKey())) { flagsChanged.add(changed.getKey()); } }

I am not sure, if Flag implements proper equals and hashcode, and the InMemoryProvider is likely used by tests only, therefore sending too many keys is likely OK; it is OK for my use-cases at least, so the "complex" solution is optional.

Missing flag keys in the changed event are a problem though.

Oh, nice catch.

Ya I think it's fine just to list all flags here. This is just a testing util.

@ryanprayogo you can mention that all flags will be put into the change notification. This is a testing util primarily, and anyway providers get to decide what constitutes "changed" for their purposes.

AndiHofi · 2024-08-22T14:59:04Z

src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java

+    }
+
+    @Test
+    void multithreadedTest() throws InterruptedException {


Please remove this test. It is likely not stable and would cause build problems for others.

It is now a thing wrapper over ConcurrentHashMap (with some event handling), there is little value in testing thread-safety of ConcurrentHashMap.

I'm not sure the test is unstable, but I do question it's value.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

sonarqubecloud · 2024-08-22T19:46:04Z

Quality Gate passed

Issues
0 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 SonarCloud

toddbaert · 2024-08-22T19:46:47Z

@ryanprayogo I've made the small changes recommended by @AndiHofi . I'm sure you could have done them, but I want to get this into the next release I was hoping to do today! Thanks for fixing this (and also for your other Java 21 fix!)

ryanprayogo requested a review from a team as a code owner August 21, 2024 02:15

ryanprayogo force-pushed the fix-1054 branch from 64af7ae to 7892900 Compare August 21, 2024 02:17

fix: Use ConcurrentHashMap for InMemoryProvider

ebcb5ce

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

ryanprayogo force-pushed the fix-1054 branch from 7892900 to ebcb5ce Compare August 21, 2024 02:21

Kavindu-Dodan approved these changes Aug 21, 2024

View reviewed changes

AndiHofi suggested changes Aug 21, 2024

View reviewed changes

toddbaert self-requested a review August 21, 2024 14:57

toddbaert requested changes Aug 21, 2024

View reviewed changes

ryanprayogo force-pushed the fix-1054 branch from 9ac078e to e1a7c79 Compare August 21, 2024 18:49

ryanprayogo added 2 commits August 21, 2024 14:54

fix: make the flags field variable final

f2dbbbb

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

chore: Use Collections.singletonList instead of Arrays.asList

ce27c39

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

ryanprayogo force-pushed the fix-1054 branch from e1a7c79 to ce27c39 Compare August 21, 2024 18:58

ryanprayogo commented Aug 21, 2024

View reviewed changes

src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java Outdated Show resolved Hide resolved

src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java Show resolved Hide resolved

toddbaert reviewed Aug 21, 2024

View reviewed changes

toddbaert approved these changes Aug 21, 2024

View reviewed changes

toddbaert requested a review from AndiHofi August 21, 2024 19:08

chore: Update javadoc and parameter name

69f3976

Signed-off-by: Ryan Prayogo <57620+ryanprayogo@users.noreply.github.com>

toddbaert reviewed Aug 22, 2024

View reviewed changes

fixup: await verify

e139de4

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert force-pushed the fix-1054 branch from cf4be29 to e139de4 Compare August 22, 2024 13:12

Merge branch 'main' into fix-1054

2375acc

AndiHofi suggested changes Aug 22, 2024

View reviewed changes

toddbaert added 2 commits August 22, 2024 15:45

fixup: remove test, key diffing

e7bdece

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Merge branch 'main' into fix-1054

b62f4ea

toddbaert merged commit b7ed041 into open-feature:main Aug 22, 2024
10 checks passed

github-actions bot mentioned this pull request Aug 22, 2024

chore(main): release 1.9.1 #996

Merged

toddbaert mentioned this pull request Aug 22, 2024

chore: invite ryanprayogo open-feature/community#393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use ConcurrentHashMap for InMemoryProvider #1057

fix: Use ConcurrentHashMap for InMemoryProvider #1057

ryanprayogo commented Aug 21, 2024

codecov bot commented Aug 21, 2024 •

edited

Loading

AndiHofi left a comment •

edited

Loading

toddbaert commented Aug 21, 2024

toddbaert left a comment •

edited

Loading

toddbaert Aug 21, 2024

ryanprayogo Aug 21, 2024

toddbaert left a comment

toddbaert Aug 22, 2024 •

edited

Loading

toddbaert Aug 22, 2024

ryanprayogo Aug 22, 2024

toddbaert Aug 22, 2024

ryanprayogo Aug 22, 2024

AndiHofi left a comment

AndiHofi Aug 22, 2024

toddbaert Aug 22, 2024 •

edited

Loading

AndiHofi Aug 22, 2024

toddbaert Aug 22, 2024

sonarqubecloud bot commented Aug 22, 2024

toddbaert commented Aug 22, 2024

fix: Use ConcurrentHashMap for InMemoryProvider #1057

fix: Use ConcurrentHashMap for InMemoryProvider #1057

Conversation

ryanprayogo commented Aug 21, 2024

Use ConcurrentHashMap for InMemoryProvider

Related Issues

Notes

Follow-up Tasks

How to test

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

AndiHofi left a comment • edited Loading

Choose a reason for hiding this comment

toddbaert commented Aug 21, 2024

toddbaert left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert left a comment

Choose a reason for hiding this comment

toddbaert Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndiHofi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 22, 2024

Quality Gate passed

toddbaert commented Aug 22, 2024

codecov bot commented Aug 21, 2024 •

edited

Loading

AndiHofi left a comment •

edited

Loading

toddbaert left a comment •

edited

Loading

toddbaert Aug 22, 2024 •

edited

Loading

toddbaert Aug 22, 2024 •

edited

Loading