From 68db68511e2460bb1f68a8cc7ae8603e321eaae5 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 24 Feb 2022 11:50:40 -0500 Subject: [PATCH 1/3] Add test for race condition in Eager Ast nodes --- .../java/com/hubspot/jinjava/Jinjava.java | 1 + .../util/EagerExpressionResolverTest.java | 61 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/main/java/com/hubspot/jinjava/Jinjava.java b/src/main/java/com/hubspot/jinjava/Jinjava.java index 1b739c15a..ef0b29ac6 100644 --- a/src/main/java/com/hubspot/jinjava/Jinjava.java +++ b/src/main/java/com/hubspot/jinjava/Jinjava.java @@ -97,6 +97,7 @@ public Jinjava(JinjavaConfig globalConfig) { TreeBuilder.class.getName(), EagerExtendedSyntaxBuilder.class.getName() ); + // eagerExpConfig.setProperty(ExpressionFactoryImpl.PROP_CACHE_SIZE, "0"); TypeConverter converter = new TruthyTypeConverter(); this.expressionFactory = new ExpressionFactoryImpl(expConfig, converter); diff --git a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java index 463d371d1..c6119ad03 100644 --- a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java +++ b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java @@ -32,6 +32,8 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicLong; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -77,6 +79,15 @@ private JinjavaInterpreter getInterpreter(boolean evaluateMapKeys) throws Except this.getClass().getDeclaredMethod("isNull", Object.class, Object.class) ) ); + jinjava + .getGlobalContext() + .registerFunction( + new ELFunctionDefinition( + "", + "sleeper", + this.getClass().getDeclaredMethod("sleeper") + ) + ); interpreter = new JinjavaInterpreter(jinjava.newInterpreter()); context = interpreter.getContext(); context.put("deferred", DeferredValue.instance()); @@ -718,12 +729,62 @@ public void itMarksDictionaryKeysAsDeferredWordsIfEvaluated() throws Exception { } } + @SuppressWarnings("SynchronizationOnLocalVariableOrMethodParameter") + @Test + public void itIsThreadSafe() throws InterruptedException { + Map map = new HashMap<>(); + map.put("bar", "first"); + AtomicLong sleepTime = new AtomicLong(500L); + context.put("map", map); + context.put("sleep_time", sleepTime); + CompletableFuture first; + synchronized (sleepTime) { + first = CompletableFuture.supplyAsync(this::appendAndSleep); + sleepTime.wait(); + } + sleepTime.set(1L); + map.put("bar", "second"); + CompletableFuture second = CompletableFuture.supplyAsync( + this::appendAndSleep + ); + CompletableFuture.allOf(first, second).join(); + assertThat(first.join().toString()) + .describedAs("First result should say 'first' and sleep for 500ms") + .isEqualTo("deferred && 'first' && 500"); + assertThat(second.join().toString()) // caching would make this say 'first' + .describedAs("Second result should say 'second' and sleep for 1ms") + .isEqualTo("deferred && 'second' && 1"); + } + + private EagerExpressionResult appendAndSleep() { + JinjavaInterpreter.pushCurrent(interpreter); + try { + return eagerResolveExpression("deferred && map.bar && sleeper()"); + } finally { + JinjavaInterpreter.popCurrent(); + } + } + public static void voidFunction(int nothing) {} public static boolean isNull(Object foo, Object bar) { return foo == null && bar == null; } + @SuppressWarnings("SynchronizationOnLocalVariableOrMethodParameter") + public static long sleeper() throws InterruptedException { + AtomicLong atomicSleepTime = (AtomicLong) JinjavaInterpreter + .getCurrent() + .getContext() + .get("sleep_time"); + long sleepTime = atomicSleepTime.get(); + synchronized (atomicSleepTime) { + atomicSleepTime.notify(); + } + Thread.sleep(sleepTime); + return sleepTime; + } + private static class Foo { private final String bar; From dad339be60eb32ff100e7936aafdfc8efbf5b4de Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 24 Feb 2022 17:22:18 -0500 Subject: [PATCH 2/3] Fix race condition by not caching AstNodes when using eager execution --- src/main/java/com/hubspot/jinjava/Jinjava.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/hubspot/jinjava/Jinjava.java b/src/main/java/com/hubspot/jinjava/Jinjava.java index ef0b29ac6..f2a9878a6 100644 --- a/src/main/java/com/hubspot/jinjava/Jinjava.java +++ b/src/main/java/com/hubspot/jinjava/Jinjava.java @@ -97,7 +97,7 @@ public Jinjava(JinjavaConfig globalConfig) { TreeBuilder.class.getName(), EagerExtendedSyntaxBuilder.class.getName() ); - // eagerExpConfig.setProperty(ExpressionFactoryImpl.PROP_CACHE_SIZE, "0"); + eagerExpConfig.setProperty(ExpressionFactoryImpl.PROP_CACHE_SIZE, "0"); TypeConverter converter = new TruthyTypeConverter(); this.expressionFactory = new ExpressionFactoryImpl(expConfig, converter); From 56cf6cafe3f48bcb61ce313688567622b1420fa2 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 24 Feb 2022 17:24:42 -0500 Subject: [PATCH 3/3] Don't need to clear eval result if we are no longer using caching for eager execution of ast nodes --- .../jinjava/el/ext/eager/EagerAstBinary.java | 8 -------- .../jinjava/el/ext/eager/EagerAstBracket.java | 10 ---------- .../jinjava/el/ext/eager/EagerAstChoice.java | 9 --------- .../jinjava/el/ext/eager/EagerAstDict.java | 17 ----------------- .../jinjava/el/ext/eager/EagerAstDot.java | 7 ------- .../el/ext/eager/EagerAstIdentifier.java | 6 ------ .../jinjava/el/ext/eager/EagerAstList.java | 9 --------- .../el/ext/eager/EagerAstMacroFunction.java | 10 ---------- .../jinjava/el/ext/eager/EagerAstMethod.java | 8 -------- .../el/ext/eager/EagerAstNamedParameter.java | 7 ------- .../jinjava/el/ext/eager/EagerAstNested.java | 7 ------- .../el/ext/eager/EagerAstNodeDecorator.java | 6 ------ .../el/ext/eager/EagerAstParameters.java | 7 ------- .../el/ext/eager/EagerAstRangeBracket.java | 15 --------------- .../jinjava/el/ext/eager/EagerAstRoot.java | 8 +------- .../jinjava/el/ext/eager/EagerAstTuple.java | 9 --------- .../jinjava/el/ext/eager/EagerAstUnary.java | 7 ------- .../jinjava/el/ext/eager/EvalResultHolder.java | 2 -- 18 files changed, 1 insertion(+), 151 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java index 3f3052a82..95d92e917 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java @@ -82,14 +82,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - left.clearEvalResult(); - right.clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java index 8558a37cd..85bd00346 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java @@ -46,16 +46,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - ((EvalResultHolder) prefix).clearEvalResult(); - if (property != null) { - ((EvalResultHolder) property).clearEvalResult(); - } - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java index 7dd4d91b1..c8c91109d 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java @@ -61,15 +61,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - question.clearEvalResult(); - yes.clearEvalResult(); - no.clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java index 2ebefa753..7711736a8 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java @@ -96,23 +96,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - - dict.forEach( - (key, value) -> { - if (key instanceof EvalResultHolder) { - ((EvalResultHolder) key).clearEvalResult(); - } - if (value instanceof EvalResultHolder) { - ((EvalResultHolder) value).clearEvalResult(); - } - } - ); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java index ed80f2284..862d9620d 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java @@ -72,13 +72,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - base.clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java index 21a7049f1..a7b631e4a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java @@ -33,12 +33,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java index 039e92bef..6ca90e4a1 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java @@ -35,15 +35,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - for (int i = 0; i < elements.getCardinality(); i++) { - ((EvalResultHolder) elements.getChild(i)).clearEvalResult(); - } - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java index 329a2fa77..e87f2e0a5 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java @@ -90,16 +90,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - params.clearEvalResult(); - for (int i = 0; i < ((AstParameters) params).getCardinality(); i++) { - ((EvalResultHolder) ((AstParameters) params).getChild(i)).clearEvalResult(); - } - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java index 865e492e1..9d6a08ee5 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java @@ -59,14 +59,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - property.clearEvalResult(); - params.clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java index 1c6748837..8d1a23c61 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java @@ -65,13 +65,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - value.clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java index 047bbef5c..317167c11 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java @@ -51,13 +51,6 @@ public String toString() { return "(...)"; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - ((EvalResultHolder) child).clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java index 68732b28a..3f876ea38 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java @@ -43,12 +43,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java index d31dfb79d..6ad81924a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java @@ -76,13 +76,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - nodes.forEach(node -> ((EvalResultHolder) node).clearEvalResult()); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java index 7b28a4927..0f4f8d4f6 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java @@ -83,21 +83,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - if (prefix != null) { - ((EvalResultHolder) prefix).clearEvalResult(); - } - if (property != null) { - ((EvalResultHolder) property).clearEvalResult(); - } - if (rangeMax != null) { - ((EvalResultHolder) rangeMax).clearEvalResult(); - } - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRoot.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRoot.java index 7eeaa4fbe..d785cd63d 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRoot.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRoot.java @@ -21,13 +21,7 @@ public void appendStructure(StringBuilder builder, Bindings bindings) { @Override public Object eval(Bindings bindings, ELContext context) { - try { - return rootNode.eval(bindings, context); - } finally { - if (rootNode instanceof EvalResultHolder) { - ((EvalResultHolder) rootNode).clearEvalResult(); - } - } + return rootNode.eval(bindings, context); } @Override diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java index 1d6af62e1..e0c4b86b4 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java @@ -57,15 +57,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - for (int i = 0; i < elements.getCardinality(); i++) { - ((EvalResultHolder) elements.getChild(i)).clearEvalResult(); - } - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java index 115789d43..ffdbdfc36 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java @@ -61,13 +61,6 @@ public void setEvalResult(Object evalResult) { hasEvalResult = true; } - @Override - public void clearEvalResult() { - evalResult = null; - hasEvalResult = false; - child.clearEvalResult(); - } - @Override public boolean hasEvalResult() { return hasEvalResult; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java index a5a0bb8e9..da8ec4247 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java @@ -17,8 +17,6 @@ public interface EvalResultHolder { void setEvalResult(Object evalResult); - void clearEvalResult(); - boolean hasEvalResult(); default Object eval(