Repository: PebbleTemplates/pebble
Pull request #185 (2016-03-18 13:46:37)

Fix ConcurrentModificationException when importing macros in nested parallels blocks (#182) by snapshotting the importedTemplates passed into EvaluationContext.
This patch fixes ConcurrentModificationException when importing macros in nested parallels blocks (#182) by snapshotting the importedTemplates passed into EvaluationContext. 

The benefit of snapshotting the templates is that this way, they don't "leak" to the outer context in case of parallel blocks which is what I would expect. Obviously, it fixes the ConcurrentModificationException as well. 

Furthermore, the patch includes a test for the issue that fails without the patch but passes with it on my system. Unfortunately, the test is done in a little roundabout way as the actual issue (the ConcurrentModificationException) is silently swallowed and doesn't fail the overall template execution (which I would consider a bug in its own right) -- hence, it just does run same same template one time without an Executor and one time with an Executor and compares the result (which isn't guaranteed to fail depending on you hardware - it does however, fail on my system everytime out of 20 runs).  


Patch:
diff --git a/src/main/java/com/mitchellbosecke/pebble/template/EvaluationContext.java b/src/main/java/com/mitchellbosecke/pebble/template/EvaluationContext.java
index a68fc461..d8d6f4af 100644
--- a/src/main/java/com/mitchellbosecke/pebble/template/EvaluationContext.java
+++ b/src/main/java/com/mitchellbosecke/pebble/template/EvaluationContext.java
@@ -11,6 +11,7 @@ package com.mitchellbosecke.pebble.template;
 import com.google.common.cache.Cache;
 import com.mitchellbosecke.pebble.cache.BaseTagCacheKey;
 import com.mitchellbosecke.pebble.extension.ExtensionRegistry;
+import java.util.ArrayList;
 
 import java.util.List;
 import java.util.Locale;
@@ -95,7 +96,7 @@ public class EvaluationContext {
         this.extensionRegistry = extensionRegistry;
         this.tagCache = tagCache;
         this.executorService = executorService;
-        this.importedTemplates = importedTemplates;
+        this.importedTemplates = importedTemplates != null ? new ArrayList<PebbleTemplateImpl>(importedTemplates) : null;
         this.scopeChain = scopeChain;
         this.hierarchy = hierarchy;
     }


Test patch:
diff --git a/src/test/java/com/mitchellbosecke/pebble/ConcurrencyTest.java b/src/test/java/com/mitchellbosecke/pebble/ConcurrencyTest.java
index 1f54002d..50a337d5 100644
--- a/src/test/java/com/mitchellbosecke/pebble/ConcurrencyTest.java
+++ b/src/test/java/com/mitchellbosecke/pebble/ConcurrencyTest.java
@@ -1,19 +1,23 @@
 /*******************************************************************************
  * This file is part of Pebble.
- * 
+ *
  * Copyright (c) 2014 by Mitchell Bösecke
- * 
+ *
  * For the full copyright and license information, please view the LICENSE
  * file that was distributed with this source code.
  ******************************************************************************/
 package com.mitchellbosecke.pebble;
 
+import com.mitchellbosecke.pebble.error.LoaderException;
 import com.mitchellbosecke.pebble.error.PebbleException;
+import com.mitchellbosecke.pebble.loader.Loader;
 import com.mitchellbosecke.pebble.loader.StringLoader;
 import com.mitchellbosecke.pebble.template.PebbleTemplate;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
 import java.io.StringWriter;
 import java.security.SecureRandom;
 import java.util.HashMap;
@@ -38,7 +42,7 @@ public class ConcurrencyTest extends AbstractTest {
         final public int b;
 
         final public int c;
-        
+
         final public String d;
 
         private TestObject(int a, int b, int c, String d) {
@@ -116,7 +120,7 @@ public class ConcurrencyTest extends AbstractTest {
      * will only compile one at a time. This test simply makes sure that if a
      * user attempts to trigger concurrent compilation everything still succeeds
      * (despite it being executed one at a time behind the scenes).
-     * 
+     *
      * @throws InterruptedException
      * @throws PebbleException
      */
@@ -205,7 +209,7 @@ public class ConcurrencyTest extends AbstractTest {
                     }
                 }
             });
-            
+
             if (totalFailed.intValue() > 0) {
                 break;
             }
@@ -218,7 +222,7 @@ public class ConcurrencyTest extends AbstractTest {
 
     /**
      * Issue #40
-     * 
+     *
      * @throws InterruptedException
      * @throws PebbleException
      */
@@ -281,4 +285,92 @@ public class ConcurrencyTest extends AbstractTest {
         es.shutdown();
         assertEquals(0, totalFailed.intValue());
     }
+
+    @Test
+    public void testConcurrentEvaluationWithImportingMacros() throws PebbleException, IOException
+    {
+        Loader<String> loader = new Loader<String>(){
+            private final String TEMPLATE =
+                  "{% import \"macro\" %}"
+                + "{{test(0)}}"
+                + "{% for i in [1,2,3,4,5,6,7,8,9,10] %}"
+                + "    {% parallel %}"
+                + "        {% import \"macro\" %}"
+                + "        {{test(i)}}"
+                + "    {% endparallel %}"
+                + "{% endfor %}";
+
+            private final String MACRO =
+                  "{% macro test(count) %}"
+                + "    test({{count}})"
+                + "{% endmacro %}";
+
+            @Override
+            public Reader getReader(String cacheKey) throws LoaderException
+            {
+                switch (cacheKey)
+                {
+                    case "template":
+                        return new StringReader(TEMPLATE);
+                    case "macro":
+                        return new StringReader(MACRO);
+                    default:
+                        throw new IllegalStateException("No such file");
+                }
+            }
+
+            @Override
+            public void setCharset(String charset)
+            {
+                throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
+            }
+
+            @Override
+            public void setPrefix(String prefix)
+            {
+                throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
+            }
+
+            @Override
+            public void setSuffix(String suffix)
+            {
+                throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
+            }
+
+            @Override
+            public String resolveRelativePath(String relativePath, String anchorPath)
+            {
+                return relativePath;
+            }
+
+            @Override
+            public String createCacheKey(String templateName)
+            {
+                return templateName;
+            }
+
+        };
+
+        PebbleEngine engine = new PebbleEngine.Builder().loader(loader).strictVariables(false).build();
+
+        PebbleTemplate template = engine.getTemplate("template");
+
+        StringWriter singleThreadResult = new StringWriter();
+
+        template.evaluate(singleThreadResult);
+
+        ExecutorService executor = Executors.newCachedThreadPool();
+
+        engine = new PebbleEngine.Builder().loader(loader).executorService(executor).strictVariables(false).build();
+
+        template = engine.getTemplate("template");
+
+        StringWriter multipleThreadResult = new StringWriter();
+
+        template.evaluate(multipleThreadResult);
+
+        executor.shutdown();
+        
+        assertEquals(singleThreadResult.toString(), multipleThreadResult.toString());
+    }
 }