From 298714ac382c21206d2bbeaa8783f4398c79f456 Mon Sep 17 00:00:00 2001
From: Simon Templer <simon@wetransform.to>
Date: Thu, 4 Jan 2024 10:50:11 +0100
Subject: [PATCH] fix: fix ConcurrentModificationException on Java 9+

Use a different concurrent map class than in the previous attempt, that
allows for recursive computeIfAbsent calls that modify a map.
---
 .../pebble/PebbleCachingEvaluator.groovy      | 37 ++++++++-
 .../pebble/PebbleCachingEvaluatorTest.groovy  | 76 ++++++++++++++++++-
 2 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/src/main/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluator.groovy b/src/main/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluator.groovy
index 393a1e6..8e1b463 100644
--- a/src/main/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluator.groovy
+++ b/src/main/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluator.groovy
@@ -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
 
@@ -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) {
@@ -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)
     }
 
@@ -186,7 +215,7 @@ public class PebbleCachingEvaluator extends AbstractPebbleEvaluator {
 
     @Override
     public Set<String> keySet() {
-      return original.keySet();
+      return original.keySet()
     }
 
     @Override
diff --git a/src/test/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluatorTest.groovy b/src/test/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluatorTest.groovy
index 20e5237..919ecab 100644
--- a/src/test/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluatorTest.groovy
+++ b/src/test/groovy/to/wetransform/gradle/swarm/config/pebble/PebbleCachingEvaluatorTest.groovy
@@ -16,6 +16,7 @@
 
 package to.wetransform.gradle.swarm.config.pebble
 
+import org.junit.Ignore
 import org.junit.Test
 
 /**
@@ -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 }}',
@@ -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 = [