Skip to content

fix: extract mutable eval state into EvalContext — fixes #90, #93#103

Open
matanzwix wants to merge 2 commits intodashjoin:mainfrom
matanzwix:fix/thread-safety-eval-context
Open

fix: extract mutable eval state into EvalContext — fixes #90, #93#103
matanzwix wants to merge 2 commits intodashjoin:mainfrom
matanzwix:fix/thread-safety-eval-context

Conversation

@matanzwix
Copy link
Copy Markdown

@matanzwix matanzwix commented Mar 28, 2026

Problem

Two related thread-safety bugs make it unsafe to cache and reuse parsed Jsonata instances:

Issue #93 — Bindings leak across instances on the same thread

The constructor sets current.set(this) on a single static ThreadLocal<Jsonata>. When a second instance is constructed on the same thread, it overwrites the first. Subsequent evaluations of the first instance can silently use the second instance's per-thread evaluator state, causing environment bindings to leak across unrelated expressions.

Issue #90$eval() returns null for nested/deep contexts

_evaluate() stores evaluation state such as input and environment as mutable instance fields during recursive evaluation. When $eval() reads that state through Jsonata.current.get(), it can observe whatever the most recent recursive _evaluate() call wrote, rather than the correct calling context for that $eval() invocation. This causes cases like $eval($.funcs.func) to return null instead of the expected result.

Both bugs stem from the same root cause: per-evaluation mutable state stored on the shared Jsonata instance and accessed through a single static ThreadLocal<Jsonata>.

Solution

Extract all per-evaluation mutable state into a lightweight EvalContext value object stored in a single static ThreadLocal<EvalContext>.

This separates:

  • the parsed expression and base configuration on Jsonata
  • the live execution state for the current evaluation

As a result, Jsonata no longer stores per-evaluation mutable state, and parsed expressions can be safely cached and reused across threads after setup.

What changed

Jsonata.java

Before After
static ThreadLocal<Jsonata> current static ThreadLocal<EvalContext> evalContext
Per-evaluation mutable state stored on Jsonata (input, timestamp, current eval state) Per-evaluation mutable state stored on EvalContext
getPerThreadInstance() + clone per thread Removed
Copy constructor Jsonata(Jsonata other) Removed
Constructor sets current.set(this) Removed
_evaluate() overwrites mutable state on this _evaluate() saves/restores state on EvalContext in try/finally
evaluate(Object, Frame) does not manage a dedicated execution context evaluate(Object, Frame) creates/restores EvalContext

Functions.java

  • All Jsonata.current.get() call sites now read from Jsonata.evalContext.get()
  • $eval(), $now(), $millis(), and lambda application now use EvalContext
  • funcApply() no longer performs redundant repeated ThreadLocal.get() calls

Design

Before:
  Jsonata instance held mutable evaluation state
  static ThreadLocal<Jsonata>
  per-thread cloning

After:
  Jsonata holds parsed expression + base configuration
  static ThreadLocal<EvalContext>
  no cloning

…n#90, dashjoin#93)

Replace the broken static ThreadLocal<Jsonata> with a lightweight
EvalContext value object on a single static ThreadLocal. The Jsonata
instance is now immutable after construction and freely shareable
across threads with no cloning.
@matanzwix
Copy link
Copy Markdown
Author

@uw4 @aeberhart could you please take a look?

@uw4
Copy link
Copy Markdown
Contributor

uw4 commented Apr 20, 2026

Thanks for the excellent contribution + providing the test cases, this is an excellent result!

However we try to minimize the # of changes/changed lines of code, and also try to stay structurally as near as possible with the original Javascript.

Thus while reproducing your work, it turned out that this is the only change required in order to fix the issues and pass all test cases (and also has no material effect on any benchmarks):

diff --git a/src/main/java/com/dashjoin/jsonata/Jsonata.java b/src/main/java/com/dashjoin/jsonata/Jsonata.java
index d60bf19..fe87c9f 100644
--- a/src/main/java/com/dashjoin/jsonata/Jsonata.java
+++ b/src/main/java/com/dashjoin/jsonata/Jsonata.java
@@ -129,7 +129,15 @@ public class Jsonata {
     Object evaluate(Symbol expr, Object input, Frame environment) {
         // Thread safety:
         // Make sure each evaluate is executed on an instance per thread
-        return getPerThreadInstance()._evaluate(expr, input, environment);
+        Jsonata _this = getPerThreadInstance();
+        Object _input = _this.input;
+        Frame _environment = _this.environment;
+        try {
+          return _this._evaluate(expr, input, environment);
+        } finally {
+            _this.input = _input;
+            _this.environment = _environment;
+        }
     }
 
     Object _evaluate(Symbol expr, Object input, Frame environment) {
  • Can you please just use this minimal change in the pull request (along with the unit tests)?
    Then I would merge it so you are preserved as commiter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants