Make stack operations use AutoCloseable for safer usage with try-with-resources

pom.xml

@@ -20,6 +20,7 @@
     <dep.plugin.jacoco.version>0.8.3</dep.plugin.jacoco.version>
     <dep.plugin.javadoc.version>3.0.1</dep.plugin.javadoc.version>
     <dep.hubspot-immutables.version>1.9</dep.hubspot-immutables.version>
+    <dep.algebra.version>1.5</dep.algebra.version>
 
 
     <basepom.test.add.opens>
@@ -101,6 +102,11 @@
         <artifactId>immutables-exceptions</artifactId>
         <version>${dep.hubspot-immutables.version}</version>
       </dependency>
+      <dependency>
+        <groupId>com.hubspot</groupId>
+        <artifactId>algebra</artifactId>
+        <version>${dep.algebra.version}</version>
+      </dependency>
     </dependencies>
   </dependencyManagement>
 
@@ -213,6 +219,10 @@
       <groupId>com.hubspot.immutables</groupId>
       <artifactId>immutables-exceptions</artifactId>
     </dependency>
+    <dependency>
+      <groupId>com.hubspot</groupId>
+      <artifactId>algebra</artifactId>
+    </dependency>
   </dependencies>
 
   <build>

src/main/java/com/hubspot/jinjava/Jinjava.java

@@ -20,6 +20,7 @@
 import com.hubspot.jinjava.el.ExtendedSyntaxBuilder;
 import com.hubspot.jinjava.el.TruthyTypeConverter;
 import com.hubspot.jinjava.el.ext.eager.EagerExtendedSyntaxBuilder;
+import com.hubspot.jinjava.interpret.AutoCloseableSupplier.AutoCloseableImpl;
 import com.hubspot.jinjava.interpret.Context;
 import com.hubspot.jinjava.interpret.FatalTemplateErrorsException;
 import com.hubspot.jinjava.interpret.InterpretException;
@@ -245,52 +246,55 @@ public RenderResult renderForResult(
       context = new Context(copyGlobalContext(), bindings, renderConfig.getDisabled());
     }
 
-    JinjavaInterpreter interpreter = globalConfig
-      .getInterpreterFactory()
-      .newInstance(this, context, renderConfig);
-    JinjavaInterpreter.pushCurrent(interpreter);
-
-    try {
-      String result = interpreter.render(template);
-      return new RenderResult(
-        result,
-        interpreter.getContext(),
-        interpreter.getErrorsCopy()
-      );
-    } catch (InterpretException e) {
-      if (e instanceof TemplateSyntaxException) {
+    try (
+      AutoCloseableImpl<JinjavaInterpreter> interpreterAutoCloseable = JinjavaInterpreter
+        .closeablePushCurrent(
+          globalConfig.getInterpreterFactory().newInstance(this, context, renderConfig)
+        )
+        .get()
+    ) {
+      JinjavaInterpreter interpreter = interpreterAutoCloseable.value();
+      try {
+        String result = interpreter.render(template);
+        return new RenderResult(
+          result,
+          interpreter.getContext(),
+          interpreter.getErrorsCopy()
+        );
+      } catch (InterpretException e) {
+        if (e instanceof TemplateSyntaxException) {
+          return new RenderResult(
+            TemplateError.fromException((TemplateSyntaxException) e),
+            interpreter.getContext(),
+            interpreter.getErrorsCopy()
+          );
+        }
+        return new RenderResult(
+          TemplateError.fromSyntaxError(e),
+          interpreter.getContext(),
+          interpreter.getErrorsCopy()
+        );
+      } catch (InvalidArgumentException e) {
+        return new RenderResult(
+          TemplateError.fromInvalidArgumentException(e),
+          interpreter.getContext(),
+          interpreter.getErrorsCopy()
+        );
+      } catch (InvalidInputException e) {
         return new RenderResult(
-          TemplateError.fromException((TemplateSyntaxException) e),
+          TemplateError.fromInvalidInputException(e),
+          interpreter.getContext(),
+          interpreter.getErrorsCopy()
+        );
+      } catch (Exception e) {
+        return new RenderResult(
+          TemplateError.fromException(e),
           interpreter.getContext(),
           interpreter.getErrorsCopy()
         );
       }
-      return new RenderResult(
-        TemplateError.fromSyntaxError(e),
-        interpreter.getContext(),
-        interpreter.getErrorsCopy()
-      );
-    } catch (InvalidArgumentException e) {
-      return new RenderResult(
-        TemplateError.fromInvalidArgumentException(e),
-        interpreter.getContext(),
-        interpreter.getErrorsCopy()
-      );
-    } catch (InvalidInputException e) {
-      return new RenderResult(
-        TemplateError.fromInvalidInputException(e),
-        interpreter.getContext(),
-        interpreter.getErrorsCopy()
-      );
-    } catch (Exception e) {
-      return new RenderResult(
-        TemplateError.fromException(e),
-        interpreter.getContext(),
-        interpreter.getErrorsCopy()
-      );
     } finally {
       globalContext.reset();
-      JinjavaInterpreter.popCurrent();
     }
   }
 

src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java

@@ -1,10 +1,13 @@
 package com.hubspot.jinjava.el.ext;
 
 import com.google.common.collect.ImmutableMap;
+import com.hubspot.algebra.Result;
+import com.hubspot.jinjava.interpret.AutoCloseableSupplier;
+import com.hubspot.jinjava.interpret.AutoCloseableSupplier.AutoCloseableImpl;
 import com.hubspot.jinjava.interpret.CallStack;
 import com.hubspot.jinjava.interpret.DeferredValueException;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
-import com.hubspot.jinjava.interpret.MacroTagCycleException;
+import com.hubspot.jinjava.interpret.TagCycleException;
 import com.hubspot.jinjava.interpret.TemplateError;
 import com.hubspot.jinjava.interpret.errorcategory.BasicTemplateErrorCategory;
 import com.hubspot.jinjava.lib.fn.MacroFunction;
@@ -18,6 +21,10 @@
 
 public class AstMacroFunction extends AstFunction {
 
+  public enum MacroCallError {
+    CYCLE_DETECTED,
+  }
+
   public AstMacroFunction(String name, int index, AstParameters params, boolean varargs) {
     super(name, index, params, varargs);
   }
@@ -37,30 +44,16 @@ public Object eval(Bindings bindings, ELContext context) {
           interpreter.getPosition()
         );
       }
-      if (!macroFunction.isCaller()) {
-        if (checkAndPushMacroStack(interpreter, getName())) {
-          return "";
-        }
+      if (macroFunction.isCaller()) {
+        return wrapInvoke(bindings, context, macroFunction);
       }
-
-      try {
-        return invoke(
-          bindings,
-          context,
-          macroFunction,
-          AbstractCallableMethod.EVAL_METHOD
-        );
-      } catch (IllegalAccessException e) {
-        throw new ELException(LocalMessages.get("error.function.access", getName()), e);
-      } catch (InvocationTargetException e) {
-        throw new ELException(
-          LocalMessages.get("error.function.invocation", getName()),
-          e.getCause()
-        );
-      } finally {
-        if (!macroFunction.isCaller()) {
-          interpreter.getContext().getMacroStack().pop();
-        }
+      try (
+        AutoCloseableImpl<Result<String, MacroCallError>> macroStackPush =
+          checkAndPushMacroStackWithWrapper(interpreter, getName()).get()
+      ) {
+        return macroStackPush
+          .value()
+          .match(err -> "", path -> wrapInvoke(bindings, context, macroFunction));
       }
     }
 
@@ -69,62 +62,104 @@ public Object eval(Bindings bindings, ELContext context) {
       : super.eval(bindings, context);
   }
 
-  public static boolean checkAndPushMacroStack(
+  private Object wrapInvoke(
+    Bindings bindings,
+    ELContext context,
+    MacroFunction macroFunction
+  ) {
+    try {
+      return invoke(bindings, context, macroFunction, AbstractCallableMethod.EVAL_METHOD);
+    } catch (IllegalAccessException e) {
+      throw new ELException(LocalMessages.get("error.function.access", getName()), e);
+    } catch (InvocationTargetException e) {
+      throw new ELException(
+        LocalMessages.get("error.function.invocation", getName()),
+        e.getCause()
+      );
+    }
+  }
+
+  public static AutoCloseableSupplier<Result<String, MacroCallError>> checkAndPushMacroStackWithWrapper(
     JinjavaInterpreter interpreter,
     String name
   ) {
     CallStack macroStack = interpreter.getContext().getMacroStack();
-    try {
-      if (interpreter.getConfig().isEnableRecursiveMacroCalls()) {
-        if (interpreter.getConfig().getMaxMacroRecursionDepth() != 0) {
-          macroStack.pushWithMaxDepth(
+    if (interpreter.getConfig().isEnableRecursiveMacroCalls()) {
+      if (interpreter.getConfig().getMaxMacroRecursionDepth() != 0) {
+        return macroStack
+          .closeablePushWithMaxDepth(
             name,
             interpreter.getConfig().getMaxMacroRecursionDepth(),
             interpreter.getLineNumber(),
             interpreter.getPosition()
+          )
+          .map(result ->
+            result.mapErr(err -> {
+              handleMacroCycleError(interpreter, name, err);
+              return MacroCallError.CYCLE_DETECTED;
+            })
           );
-        } else {
-          macroStack.pushWithoutCycleCheck(
+      } else {
+        return macroStack
+          .closeablePushWithoutCycleCheck(
             name,
             interpreter.getLineNumber(),
             interpreter.getPosition()
-          );
-        }
-      } else {
-        macroStack.push(name, -1, -1);
-      }
-    } catch (MacroTagCycleException e) {
-      int maxDepth = interpreter.getConfig().getMaxMacroRecursionDepth();
-      if (maxDepth != 0 && interpreter.getConfig().isValidationMode()) {
-        // validation mode is only concerned with syntax
-        return true;
+          )
+          .map(Result::ok);
       }
-
-      String message = maxDepth == 0
-        ? String.format("Cycle detected for macro '%s'", name)
-        : String.format(
-          "Max recursion limit of %d reached for macro '%s'",
-          maxDepth,
-          name
-        );
-
-      interpreter.addError(
-        new TemplateError(
-          TemplateError.ErrorType.WARNING,
-          TemplateError.ErrorReason.EXCEPTION,
-          TemplateError.ErrorItem.TAG,
-          message,
-          null,
-          e.getLineNumber(),
-          e.getStartPosition(),
-          e,
-          BasicTemplateErrorCategory.CYCLE_DETECTED,
-          ImmutableMap.of("name", name)
-        )
+    }
+    return macroStack
+      .closeablePush(name, -1, -1)
+      .map(result ->
+        result.mapErr(err -> {
+          handleMacroCycleError(interpreter, name, err);
+          return MacroCallError.CYCLE_DETECTED;
+        })
       );
+  }
 
-      return true;
+  private static void handleMacroCycleError(
+    JinjavaInterpreter interpreter,
+    String name,
+    TagCycleException e
+  ) {
+    int maxDepth = interpreter.getConfig().getMaxMacroRecursionDepth();
+    if (maxDepth != 0 && interpreter.getConfig().isValidationMode()) {
+      // validation mode is only concerned with syntax
+      return;
     }
-    return false;
+
+    String message = maxDepth == 0
+      ? String.format("Cycle detected for macro '%s'", name)
+      : String.format("Max recursion limit of %d reached for macro '%s'", maxDepth, name);
+
+    interpreter.addError(
+      new TemplateError(
+        TemplateError.ErrorType.WARNING,
+        TemplateError.ErrorReason.EXCEPTION,
+        TemplateError.ErrorItem.TAG,
+        message,
+        null,
+        e.getLineNumber(),
+        e.getStartPosition(),
+        e,
+        BasicTemplateErrorCategory.CYCLE_DETECTED,
+        ImmutableMap.of("name", name)
+      )
+    );
+  }
+
+  @Deprecated
+  public static boolean checkAndPushMacroStack(
+    JinjavaInterpreter interpreter,
+    String name
+  ) {
+    return checkAndPushMacroStackWithWrapper(interpreter, name)
+      .dangerouslyGetWithoutClosing()
+      .match(
+        err -> true, // cycle detected
+        ok -> false // no cycle
+      );
   }
 }