Skip to content

Commit

Permalink
fix: fix ConcurrentModificationException on Java 9+
Browse files Browse the repository at this point in the history
Use a different concurrent map class than in the previous attempt, that
allows for recursive computeIfAbsent calls that modify a map.
  • Loading branch information
stempler committed Jan 4, 2024
1 parent ec09304 commit 298714a
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import java.util.AbstractMap.SimpleEntry
import java.util.Collection
import java.util.Map
import java.util.Set
import java.util.concurrent.ConcurrentHashMap
import java.util.concurrent.ConcurrentSkipListMap
import java.util.function.Function
import java.util.stream.Collectors

Expand Down Expand Up @@ -64,20 +66,41 @@ public class PebbleCachingEvaluator extends AbstractPebbleEvaluator {

private final Map<String, Object> evaluated

private final ThreadLocal<Set<Object>> evaluating = new ThreadLocal<Set<Object>>() {
@Override
protected Set<Object> initialValue() {
return new HashSet<Object>();
}
}

private final PebbleCachingConfig root

PebbleCachingConfig(Map<String, Object> original, PebbleCachingConfig root) {
if (original instanceof PebbleCachingConfig) throw new IllegalStateException('Cannot wrap a PebbleCachingConfig (would result in multiple evaluations)')

this.original = original
this.evaluated = new HashMap<>()
/*
* Use concurrent skip list map to avoid ConcurrentModificationException that can happen if
* computeIfAbsent is called recursively or from different threads (HashMap since Java 9) or
* running into a deadlock (when using ConcurrentHashMap).
*/
this.evaluated = new ConcurrentSkipListMap<>()
this.root = root
}

private Object evaluate(Object key) {
def value = original.get(key)
// add to set of keys being evaluated so we can detect attempts to get the same key in the same thread (loop)
boolean wasNew = evaluating.get().add(key)
if (!wasNew) {
throw new IllegalStateException("Evaluation of key $key results in an evaluation loop")
}
try {
def value = original.get(key)

return evaluateObject(value)
return evaluateObject(value)
} finally {
evaluating.get().remove(key)
}
}

private def evaluateObject(Object value) {
Expand Down Expand Up @@ -161,6 +184,12 @@ public class PebbleCachingEvaluator extends AbstractPebbleEvaluator {
@CompileStatic(TypeCheckingMode.SKIP)
@Override
public Object get(Object key) {
boolean isBeingEvaluated = evaluating.get().contains(key)
if (isBeingEvaluated) {
// Note: we need to do this check before computeIfAbsent is called because computeIfAbsent
// may otherwise stall forever waiting for the other evaluations to be complete
throw new IllegalStateException("Evaluation of key $key results in an evaluation loop")
}
return evaluated.computeIfAbsent(key, this.&evaluate)
}

Expand All @@ -186,7 +215,7 @@ public class PebbleCachingEvaluator extends AbstractPebbleEvaluator {

@Override
public Set<String> keySet() {
return original.keySet();
return original.keySet()
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package to.wetransform.gradle.swarm.config.pebble

import org.junit.Ignore
import org.junit.Test

/**
Expand Down Expand Up @@ -47,7 +48,7 @@ class PebbleCachingEvaluatorTest extends ConfigEvaluatorTest<PebbleCachingEvalua
assert evaluated == expected
}

@Test(expected = StackOverflowError) //XXX can we improve this, e.g. detect loops and/or provide useful information to the user?
@Test(expected = IllegalStateException)
void testValueLoop() {
def config = [
foo: '{{ bar }}',
Expand All @@ -59,6 +60,79 @@ class PebbleCachingEvaluatorTest extends ConfigEvaluatorTest<PebbleCachingEvalua
evaluated.bar
}

@Test
void testValueSameMap() {
def config = [
foo: 'Test',
blub: '{{ foo }}',
bar: '{{ blub }}'
]

def evaluated = eval.evaluate(config)

def expected = [
foo: 'Test',
blub: 'Test',
bar: 'Test'
]

assert evaluated == expected
}

@Test
void testValueNestedCrossRef() {
def config = [
foo: 'Test',
blub: [
no1: '{{ foo }}',
no2: '{{ blub.no1 }}',
no3: [
np1: '{{ blub.no1 }}',
np2: '{{ blub.no3.np1 }}'
]
],
bar: '{{ blub.no1 }}'
]

def evaluated = eval.evaluate(config)

def expected = [
foo: 'Test',
blub: [
no1: 'Test',
no2: 'Test',
no3: [
np1: 'Test',
np2: 'Test'
]
],
bar: 'Test'
]

assert evaluated == expected
}

@Test
void testEvaluationOrder() {
def config = [
proxy: [
use_paths: "{{ not proxy.no_proxy }}",
no_proxy: true
]
]

def evaluated = eval.evaluate(config)

assert evaluated.proxy.no_proxy == true
assert evaluated.proxy.use_paths == false

// other evaluation order
evaluated = eval.evaluate(config)

assert evaluated.proxy.use_paths == false
assert evaluated.proxy.no_proxy == true
}

@Test
void testValueNested2() {
def config = [
Expand Down

0 comments on commit 298714a

Please sign in to comment.