Merge pull request #829 from HubSpot/fix-race-condition

jasmith-hs · web-flow · commit 8eaf40ee0549 · 2022-02-25T09:16:46.000-05:00
[Eager Execution] Fix race condition in deferred AstNode evaluation
diff --git 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/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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -17,8 +17,6 @@ public interface EvalResultHolder {
 
   void setEvalResult(Object evalResult);
 
-  void clearEvalResult();
-
   boolean hasEvalResult();
 
   default Object eval(
diff --git 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<String, String> map = new HashMap<>();
+    map.put("bar", "first");
+    AtomicLong sleepTime = new AtomicLong(500L);
+    context.put("map", map);
+    context.put("sleep_time", sleepTime);
+    CompletableFuture<EagerExpressionResult> first;
+    synchronized (sleepTime) {
+      first = CompletableFuture.supplyAsync(this::appendAndSleep);
+      sleepTime.wait();
+    }
+    sleepTime.set(1L);
+    map.put("bar", "second");
+    CompletableFuture<EagerExpressionResult> 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;
 
