Skip to content
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

fix: ConcurrentModificationException on flag config change java 9 #954

Merged
merged 1 commit into from
Sep 17, 2024
Merged

fix: ConcurrentModificationException on flag config change java 9 #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 2 additions & 4 deletions .../main/java/dev/openfeature/contrib/providers/flagd/resolver/process/model/FlagParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ public class FlagParser {
private static final String FLAG_KEY = "flags";
private static final String EVALUATOR_KEY = "$evaluators";
private static final String REPLACER_FORMAT = "\"\\$ref\":(\\s)*\"%s\"";

private static final ObjectMapper MAPPER = new ObjectMapper();
private static final Map<String, Pattern> PATTERN_MAP = new HashMap<>();
Copy link
Member Author

Choose a reason for hiding this comment

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

This field presents a concurrency issue, since multiple threads can possible parse at the same time, and computeIfAbsent can throw an concurrent-modification-exception in Java 9+


private static JsonSchema SCHEMA_VALIDATOR;

private FlagParser() {
Expand Down Expand Up @@ -100,6 +97,7 @@ public static Map<String, FeatureFlag> parseString(final String configuration, b

private static String transposeEvaluators(final String configuration) throws IOException {
try (JsonParser parser = MAPPER.createParser(configuration)) {
final Map<String, Pattern> patternMap = new HashMap<>();
final TreeNode treeNode = parser.readValueAsTree();
final TreeNode evaluators = treeNode.get(EVALUATOR_KEY);

Expand All @@ -120,7 +118,7 @@ private static String transposeEvaluators(final String configuration) throws IOE

// then derive pattern
final Pattern reg_replace =
PATTERN_MAP.computeIfAbsent(replacePattern, s -> Pattern.compile(replacePattern));
patternMap.computeIfAbsent(replacePattern, s -> Pattern.compile(replacePattern));

// finally replace all references
replacedConfigurations = reg_replace.matcher(replacedConfigurations).replaceAll(replacer);
Expand Down
7 changes: 4 additions & 3 deletions .../java/dev/openfeature/contrib/providers/flagd/resolver/process/storage/FlagStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.junit.jupiter.api.Test;

import java.time.Duration;
import java.util.HashSet;
import java.util.Map;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
Expand Down Expand Up @@ -98,9 +99,9 @@ public void changedFlags() throws Exception {
Map<String, FeatureFlag> expectedChangedFlags =
FlagParser.parseString(getFlagsFromResource(VALID_LONG),true);
Copy link
Member Author

Choose a reason for hiding this comment

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

This call would intermittently fail in Java 9+ due to concurrent modification exceptions.

expectedChangedFlags.remove("myBoolFlag");
// flags changed from initial VALID_SIMPLE flag
Assert.assertEquals(expectedChangedFlags.keySet().stream().collect(Collectors.toList()),
storageStateDTOS.take().getChangedFlagsKeys());
// flags changed from initial VALID_SIMPLE flag, as a set because we don't care about order
Assert.assertEquals(expectedChangedFlags.keySet().stream().collect(Collectors.toSet()),
Copy link
Member Author

@toddbaert toddbaert Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This assert would inconsistently fail because of ordering inconsistencies.

new HashSet<>(storageStateDTOS.take().getChangedFlagsKeys()));
}

}