Merge pull request #746 from seadowg/synchronized

Protect caches from being modified concurrently
getodk · Feb 20, 2024 · a340649 · a340649
2 parents e51ef0f + 09a8a95
commit a340649
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 18 deletions.
diff --git a/src/main/java/org/javarosa/core/model/ComparisonExpressionCacheFilterStrategy.java b/src/main/java/org/javarosa/core/model/ComparisonExpressionCacheFilterStrategy.java
@@ -56,7 +56,10 @@ public List<TreeReference> filter(@NotNull DataInstance sourceInstance, @NotNull
  }
  }
 
- private List<TreeReference> getCachedEvaluations(@NotNull Supplier<List<TreeReference>> next, String key) {
+ /**
+ * Synchronized to prevent two or more threads from modifying {@link #cachedEvaluations} at once
+ */
+ private synchronized List<TreeReference> getCachedEvaluations(@NotNull Supplier<List<TreeReference>> next, String key) {
  if (cachedEvaluations.containsKey(key)) {
  return cachedEvaluations.get(key);
  } else {

diff --git a/src/main/java/org/javarosa/core/model/EqualityExpressionIndexFilterStrategy.java b/src/main/java/org/javarosa/core/model/EqualityExpressionIndexFilterStrategy.java
@@ -38,9 +38,7 @@ public List<TreeReference> filter(@NotNull DataInstance sourceInstance, @NotNull
  XPathEqExpr original = (XPathEqExpr) candidate.getOriginal();
  if (original.isEqual()) {
  String section = nodeSet + candidate.getNodeSide().toString();
- if (!index.contains(section)) {
- buildIndex(sourceInstance, candidate, children, evaluationContext, section);
- }
+ buildIndexIfNeeded(sourceInstance, candidate, children, evaluationContext, section);
 
  Object absoluteValue = candidate.evalContextSide(sourceInstance, evaluationContext);
  return index.lookup(section, absoluteValue.toString());
@@ -52,16 +50,24 @@ public List<TreeReference> filter(@NotNull DataInstance sourceInstance, @NotNull
  }
  }
 
- private void buildIndex(DataInstance sourceInstance, CompareToNodeExpression predicate, List<TreeReference> children, EvaluationContext evaluationContext, String section) {
- for (int i = 0; i < children.size(); i++) {
- TreeReference child = children.get(i);
-
- Measure.log("IndexEvaluation");
- String relativeValue = predicate.evalNodeSide(sourceInstance, evaluationContext, child, i).toString();
- index.add(section, relativeValue, child);
+ /**
+ * Synchronized to prevent two or more threads from modifying the index at once
+ */
+ private synchronized void buildIndexIfNeeded(DataInstance sourceInstance, CompareToNodeExpression predicate, List<TreeReference> children, EvaluationContext evaluationContext, String section) {
+ if (!index.contains(section)) {
+ for (int i = 0; i < children.size(); i++) {
+ TreeReference child = children.get(i);
+
+ Measure.log("IndexEvaluation");
+ String relativeValue = predicate.evalNodeSide(sourceInstance, evaluationContext, child, i).toString();
+ index.add(section, relativeValue, child);
+ }
  }
  }
 
+ /**
+ * Non thread safe index for tree references based on nested string keys (a "section" and an "item").
+ */
  private static class InMemTreeReferenceIndex {
 
  private final Map<String, Map<String, List<TreeReference>>> map = new HashMap<>();
@@ -70,22 +76,22 @@ public boolean contains(String section) {
  return map.containsKey(section);
  }
 
- public void add(String section, String key, TreeReference reference) {
+ public void add(String section, String item, TreeReference reference) {
  if (!map.containsKey(section)) {
  map.put(section, new HashMap<>());
  }
 
  Map<String, List<TreeReference>> sectionMap = map.get(section);
- if (!sectionMap.containsKey(key)) {
- sectionMap.put(key, new ArrayList<>());
+ if (!sectionMap.containsKey(item)) {
+ sectionMap.put(item, new ArrayList<>());
  }
 
- sectionMap.get(key).add(reference);
+ sectionMap.get(item).add(reference);
  }
 
- public List<TreeReference> lookup(String section, String key) {
- if (map.containsKey(section) && map.get(section).containsKey(key)) {
- return map.get(section).get(key);
+ public List<TreeReference> lookup(String section, String item) {
+ if (map.containsKey(section) && map.get(section).containsKey(item)) {
+ return map.get(section).get(item);
  } else {
  return emptyList();
  }