Reject concurrent Engine use

[sebastienros]

Summary

Fail fast when one Engine is used by concurrent host operations.

Details

• Adds non-blocking Engine ownership across execution, mutation, modules, debugger state, conversion, and event-loop entry points.
• Preserves synchronous same-thread callback re-entry, top-level async thread handoff, thread-safe background completion enqueueing, and serialized cross-thread dispatch of JavaScript callbacks converted to CLR delegates.
• Reserves an Engine for the full lifetime of returned async Tasks; nested async entry from an active Engine callback is rejected before work starts.
• Keeps debugger breakpoint administration independently thread-safe and documents pooling, disposal, and remaining direct-object risks.
• Includes the threat-model mitigation originally developed on Add initial threat model for untrusted scripts #3030; after that PR landed, this PR was rebased and now targets main directly.

Linked issue

N/A

Test plan

• Added runtime and public-contract concurrency tests, including delayed callbacks, synchronous and async cross-thread callback handoff, params-array callbacks, ownership-release races, manual-promise handoff, and recovery after rejection
• Multi-target dotnet build Jint/Jint.csproj -c Release: 0 warnings, 0 errors
• Jint.Tests.PublicInterface Release: 1796 passed, 9 skipped
• JINT_HOST_CONTRACT_VERIFICATION=1 Jint.Tests.PublicInterface Release: 1800 passed, 5 skipped
• Jint.Tests Release: 7794 passed, 4 skipped, excluding the timezone-sensitive year-10000 Date test previously confirmed failing on the base
• git diff --check origin/main...HEAD
• Prior default-job A/B measurements found no attributable execution regression; Engine construction increased by approximately 30 bytes from ownership state

Breaking change?

No public API signatures change. Concurrent Engine use was already unsupported; it now throws InvalidOperationException deterministically instead of racing or silently serializing. Authorized callbacks belonging to the active operation may transfer ownership one turn at a time, while unrelated callers still fail immediately. Hosts must await async Engine operations before reuse or disposal, and async Engine entry from inside an active callback is rejected.

[sebastienros]

A lock never hurt anyone :/ ... let's ask your benchmarking agent about the impact. And forget about it if it has any.

[lahma]

I validated this branch (c4eddf6) against its base (a1a2343) in two worktrees: full suites, targeted probes for each ownership transition, and an A/B benchmark pass in four run orientations.

The direction is right and most of the machinery holds up. But the cross-thread host-callback path is broken in three independent ways, and none of them is covered by the new tests — all 17 HostEngineConcurrencyTests block the engine thread inside a callback, so none of them ever reaches EnterTransferredHostCall. That is why CI is green.

Suites (PR head)

Suite	Result
Jint.Tests	5761 / 0 (net10.0), 5676 / 0 (net472)
Jint.Tests.PublicInterface	1505 / 0 (net10.0), 1504 / 0 (net472)
Jint.Tests.PublicInterface, JINT_HOST_CONTRACT_VERIFICATION=1	1509 / 0, 1508 / 0
Jint.Tests.CommonScripts	28 / 0 both TFMs
Jint.Tests.Test262	102,326 passed / 0 failed, 342 skipped
HostEngineConcurrencyTests × 20 consecutive runs	20/20 — not flaky
dotnet build -c Release Jint/Jint.csproj	all 5 TFMs, 0 warnings

---

Blocking

1. A host callback on a foreign thread permanently bricks the engine

Jint/Engine.cs:215. EnterTransferredHostCall has five exits; four return
new HostCallScope(this, callback ? owner : null). The one a transferred callback actually takes —
the success tail after AcquireHostCall — drops the callback owner:

_ownerDepth = 1;
_ownerToken = owner;
return new HostCallScope(this);      // <-- callback owner lost

So HostCallScope.Dispose never calls ReleaseHostCallbackAdmission, and EnterHostCallback
releases the admission only in its catch. Every admitted cross-thread callback leaks one
_hostCallbackAdmission. ReleaseAsyncHostOperation (Engine.cs:361) then sees
_hostCallbackAdmission > 0, parks _asyncReleasePending and returns without clearing
_asyncOwner — and nothing else ever clears it. The engine is dead: every later entry throws
"already in use".

The shape is ordinary — a JS callback handed to a CLR API that runs it on a pool thread:

var engine = new Engine();
engine.SetValue("schedule", new Func<Func<JsValue, JsValue[], JsValue>, Task<int>>(
    cb => Task.Run(() => { cb(JsValue.Undefined, []); return 1; })));

var value = await engine.EvaluateAsync("(async () => { await schedule(() => 1); return 42; })()");
value.AsNumber().Should().Be(42);        // passes — the async operation itself succeeds

engine.Evaluate("1 + 1").AsNumber().Should().Be(2);
// InvalidOperationException: This Engine is already in use by another thread or has an
// asynchronous operation in progress.

With a 50 ms delay on the pool thread (so the admission window is certainly open) this is
25/25 runs bricked, _hostCallbackAdmission == 1 left behind. With no delay, 63/100 brick and
the other 37 hit finding 2. There is no benign outcome.

Supplying the missing argument fixes this half — verified, 25/25 → all usable:

     _ownerDepth = 1;
     _ownerToken = owner;
-    return new HostCallScope(this);
+    return new HostCallScope(this, callback ? owner : null);

2. The admission window is a timing window, so the same callback is spuriously rejected

Jint/Engine.cs:166. _hostCallbackAdmission is non-zero only while
AwaitPromiseSettlementAsync is inside await eventLoop.WaitForEventAsync(...). A callback whose
Interlocked.Increment lands anywhere else during the same pending async operation sees == 1,
concludes there was no window to claim, falls through to EnterHostCall() and throws — even though
the engine is idle and no other thread holds it.

Because the throw happens inside the host's Task, it comes back as a rejection of the awaited JS
promise. In the repro above, with no artificial delay, 19–37% of runs fail with
PromiseRejectedException wrapping "already in use". It survives the fix for finding 1 (81 usable /
19 rejected with the patch applied), so it needs its own answer: whether a callback may be admitted
should be a function of state — is anyone executing? — not of whether the awaiter has reached its
await yet.

3. A synchronous cross-thread hand-off inside a host method now throws

A host method that dispatches the callback to another thread and blocks on it — the
dispatcher / UI-thread / render-thread shape — is strictly serialized: exactly one thread touches the
engine at a time, and the engine thread is parked inside the host method throughout. Works on the
base branch, throws here:

engine.SetValue("onOtherThread", new Func<Func<int, int>, int>(cb =>
{
    var result = 0;
    var t = new Thread(() => result = cb(41));
    t.Start();
    t.Join();
    return result;
}));

engine.Evaluate("onOtherThread(x => x + 1)");
// base: 42        head: InvalidOperationException

Ownership is keyed on thread identity rather than on mutual exclusion, so a provably-serialized
hand-off is rejected. It need not be — the owner thread is blocked and _ownerDepth is known — but
if that is the intended contract it should be stated outright, because it silently breaks working
embeddings. Note the exception surfaces on the callback thread, where a host that spawned a bare
Thread has no handler at all.

---

Behaviour changes missing from "Breaking change?"

All four verified as fails-on-head / passes-on-base:

1. ExecuteAsync and Modules.ImportAsync stopped being async methods, so parse errors and
   top-level throws now propagate synchronously instead of arriving as a faulted Task. A host
   writing var t = engine.ExecuteAsync(src); try { await t; } catch { … } stops catching.
2. Dispose() can throw. Fine as a contract, awkward as a finally: inside
   using var engine = new Engine(); with an outstanding async operation, the
   InvalidOperationException from Dispose replaces the exception that was already unwinding,
   and the recent-wrapper cache is never released. Worth considering fail-fast on use but
   best-effort on teardown.
3. ManualPromise continuations no longer drain inline unless the settling thread is the thread
   that called RegisterPromise. TryEnterHostCall already proves exclusive ownership by itself;
   the extra registrationThreadId comparison is what changes behaviour for a legitimate host that
   registers on one thread and then drives the engine exclusively from another (the pooled-engine
   shape) — Resolve silently stops running continuations and the host now needs
   Advanced.ProcessTasks(). If the narrowing is deliberate it belongs in the README paragraph.
4. Engine.Advanced.RegisterPromise() now takes ownership, so registering a promise from a
   background thread while script runs starts throwing.

---

Confirmed good

• The DefaultTypeConverter binder change fixes a real pre-existing cross-engine bug.
  _targetBinderDelegateCache is static and keyed on the AST, so with a Prepared<Script> shared
  across engines the binder built for engine A baked Expression.Constant(_engine) and then
  converted engine B's callback arguments with engine A. Isolated probe — fails on base, passes here:

  var prepared = Engine.PrepareScript("capture(x => x instanceof Object)");
  // engineA and engineB both Execute(prepared), each capturing its callback
  callbackA(new object()).Should().BeTrue();
  callbackB(new object()).Should().BeTrue();   // base: False

  That deserves its own PR — it is unrelated to concurrency and would be easy to lose in a revert.

• BreakPointCollection becoming independently thread-safe, and the ModuleImportOperation
  publication ordering (_completed written last), are both straightforwardly right.

• The trimming/AOT concern I had about engineType.GetMethod("EnterHostCallback", NonPublic) does
  not bite: Function.CallFromHost calls it directly, so the trimmer keeps it. A fully trimmed
  (TrimMode=full, Jint not rooted) self-contained publish converts a JS function to a CLR
  delegate correctly. Still a latent fragility; a nameof-based lookup or [DynamicDependency]
  would cost nothing.

---

Performance

Base = a1a2343, head = c4eddf6, default job, serial, each run from its own worktree. Four
independent A/B pairs per suite, alternating which side runs first; a row counts only if it moves the
same way in every pair.

Gate — passes. SunSpider, 26 rows × 4 pairs: no row regresses in all four pairs. Suite
medians −0.36% / +0.06% / −0.67% / +0.09%. Two rows that looked like regressions after two pairs
(bitops-nsieve-bits, regexp-dna) did not survive the third and fourth. Dromaeo, 24 rows × 2
pairs: the suite mean itself flips sign between orientations (−1.16% → +1.61%), i.e. cross-run drift
exceeds the effect; nothing attributable.

Interop, 4 pairs each (median of the four):

Row	median Δ	note
InteropBulkConversion.JsArrayToIntArray	−7.6%	alloc 123.22 → 115.40 KB
InteropBulkConversion.JsArrayToListOfInt	−4.2%	alloc 119.97 → 112.15 KB
FunctionInvocation.Warm_Apply	−1.9%
FunctionInvocation.Warm_Call / Warm_BindThenCall	−1.0% / −1.1%
ListInterop.Filter / Indexing	−0.3% / −2.0%	signs disagree across pairs
EngineConstruction.BuildEngine	flat (584.0 → 583.6 ns)	alloc 7.77 → 7.80 KB
FunctionInvocation.Warm_Bind	+4.4%	337.0 → 352.8 ns
ListInterop.HasProperty	+6.1%	1.831 → 1.950 µs

The two rows that do move consistently are not attributable to the guard, and I would not treat
them as a regression in the changed code: their siblings carry the identical guard and either don't
move or move the other way. Most tellingly, Warm_BindThenCall does everything Warm_Bind does
and more, and got faster by a larger absolute margin (−41.6 ns) than Warm_Bind got slower
(+15.8 ns). Same for HasProperty against Filter/Indexing in the same class. This is the
signature of the six new Engine instance fields shifting field offsets and code layout, not of
added work — reproducible within a build, but not a cost the guard imposes.

The one clearly attributable allocation change is +30 B per Engine (BuildEngine
7.77 → 7.80 KB) — the six new fields. Allocation is byte-identical everywhere else except
InteropBulkConversion, which allocates 7.8 KB less per op and runs 4–8% faster in all four pairs.

One note on method: the figure quoted in the PR body (EvaluateSimple 584.2 vs 585.0 ns) is a single
pair, and that row's StdDev reached 25.7 ns in my runs — not enough on its own to support a
conclusion, though the conclusion it reaches (flat) does match mine.

InteropMethodDispatchBenchmark was dropped deliberately: its own StdDev is ~5% per row, so it
cannot resolve an effect of this size, and quoting it would be quoting noise. No benchmark covers the
JS-function-as-CLR-delegate direction (Function.ToObject → CallFromHost → the try/finally-wrapped
compiled body), so that surface is unmeasured.

---

Smaller notes

• _ownershipReleased (ManualResetEventSlim) is never disposed; Engine.Dispose doesn't release it.
• AcquireHostCall waits without a timeout, so transferred callbacks are blocked, not failed fast —
  worth reconciling with the "non-blocking" framing in the summary.
• ReleaseHostCallbackAdmission writes _ownerToken = null from a foreign thread while the owner
  thread reads it non-volatilely in EnterHostCall.
• ObjectInstance.ToObject's ObjectClass.Function branch now discards its ICallable pattern
  variable and re-casts inside CallFromHost.

[lahma]

The ownership protocol is well thought out — same-thread re-entry, async reservation with thread handoff, the admission window for background callbacks, and the fail-fast semantics are all coherent, and the test coverage for the rejection paths is thorough. One path is broken, though, and it permanently bricks the engine in what is probably the single most common async embedding pattern, so I have to request changes.

Blocker: callback admission token leaks on the cross-thread transfer path

EnterHostCallback increments _hostCallbackAdmission and documents that "a larger count keeps the transfer token alive until this callback's scope exits". Two of the three success branches in EnterTransferredHostCall honour that by constructing the scope with callback ? owner : null, but the normal cross-thread branch — the one an actual background callback takes — returns new HostCallScope(this) and drops the token:

    _ownerDepth = 1;
    _ownerToken = owner;
    return new HostCallScope(this);   // <- loses the admission token when callback == true

The count never returns to zero, so ReleaseAsyncHostOperation (and the sync drain's ReleaseHostCallbackAdmission) defers the release forever and _asyncOwner stays set. Every subsequent public entry then throws InvalidOperationException — the engine is permanently unusable.

Repro (the canonical setTimeout bridge; passes on main, fails on this branch — first operation returns 42, the engine is dead afterwards):

var engine = new Engine();
engine.SetValue("later", new Action<Action>(cb => Task.Delay(50).ContinueWith(_ => cb())));

var result = engine.Evaluate("new Promise(r => later(() => r(42)))").UnwrapIfPromise();
result.AsNumber().Should().Be(42);       // passes

engine.Evaluate("1 + 1");                // throws: "This Engine is already in use by another thread
                                         //  or has an asynchronous operation in progress..."

The fix is the same one the other two branches already have:

    _ownerDepth = 1;
    _ownerToken = owner;
    return new HostCallScope(this, callback ? owner : null);

I verified locally that with this change the repro passes and all 17 HostEngineConcurrencyTests stay green. Please also add the repro above as a regression test — the suite currently exercises EnterHostCallback only in situations where the transfer is rejected (engine actively owned, cross-engine binder); nothing covers a background callback that successfully claims a suspended engine, which is exactly the scenario EnterTransferredHostCall(owner, callback: true) exists for and the one this PR's summary calls out as preserved ("thread-safe background completion", "top-level async thread handoff").

Not blocking

• The same pattern through EvaluateAsync (instead of the blocking UnwrapIfPromise) times out after 10 s with PromiseTimeout because a bare r(42) settle from the callback thread enqueues nothing and never wakes WaitForEventAsync. That reproduces identically on main, so it is pre-existing and not this PR's problem — but it may be worth a follow-up issue, since the fixed callback path makes the async variant look like it should work.
• The PR body still says "Stacked on #3030; this PR should merge into initial-threat-model" — stale now that the base is main.

[sebastienros]

Addressed the changes-requested reviews in 8a10ea1a6.

• Cross-thread callback admission is now state-based and balanced on every path, with close/admit serialized under the operation token so async completion cannot leak or prematurely release ownership.
• CLR delegate and reflected host calls yield only when they actually receive an authorized JavaScript callback, preserving synchronous dispatcher/UI-thread handoff and delayed async callbacks without adding work to ordinary interop calls.
• Added regressions for the reported delayed Action<Action> + blocking UnwrapIfPromise repro, immediate and delayed EvaluateAsync callbacks, synchronous delegate/reflection dispatch, params-array callbacks, same-thread re-entry, and post-completion engine recovery.
• Restored faulted-Task behavior for initial ExecuteAsync and ImportAsync failures, and restored inline ManualPromise draining after an exclusive thread handoff.
• Dispose remains intentionally fail-fast while an operation owns the engine because concurrent wrapper-cache teardown is unsafe; the README now calls out its exception-unwinding impact. RegisterPromise remains guarded because registration mutates engine-owned promise/event-loop state, while its settle functions remain thread-safe.
• Kept the prepared-script binder correction because operation authorization must be attached to the target function's engine; the reflection lookups now use nameof.
• Authorized transfer waits are now explicitly documented. _ownershipReleased remains engine-lifetime state: Dispose is reusable teardown rather than terminal lifetime, and retiring the signal can race already-admitted waiters.

GitHub retargeted the PR to main after #3030 landed; I rebased the complete mitigation stack onto current origin/main and updated the PR body. Release validation on the rebased stack: all five Jint targets build with 0 warnings; PublicInterface 1796 passed/9 skipped; host-contract verification 1800 passed/5 skipped; runtime 7794 passed/4 skipped excluding the same timezone-sensitive year-10000 test previously reproduced on the base; git diff --check passes.

[lahma]

Re-verified the reworked ownership protocol on 8a10ea1a6:

• The admission leak is fixed on every path: the cross-thread transfer success path now carries the callback owner into HostCallScope, both bail-out paths in EnterTransferredHostCall do too, and the throw path releases in a catch — admission is balanced everywhere. Close/admit serialized under the operation-token lock closes the race with completion.
• DelayedCrossThreadCallbackReleasesBlockingPromiseOwnership is exactly the reported repro (delayed Action<Action> bridge + blocking UnwrapIfPromise), including the post-operation assertion that the engine still works.
• Hand-traced sync blocking drain, same-thread dispatcher reentry, nested suspensions, async operation lifetime, and straggler callbacks at completion — no deadlock (the resume waiter holds no engine ownership while it waits on the admission drain, and the last release always pulses).
• Local verification on this head: PublicInterface 1796/1565 (net10/net472) green, host-contract verification leg 1800/1569 green, Jint.Tests 7795/6663 green, and 10× stress iterations of HostEngineConcurrencyTests green on both TFMs.

One documented edge to keep in mind (not blocking, pre-existing in the earlier revision too): during a synchronous blocking drain, a background callback that lands while the loop is actively running jobs — rather than parked in WaitForWork — is rejected with the concurrent-use error instead of waiting for the next window. Async operations don't have that window since the reservation spans the whole operation. It's a clean rejection with no state mutated, consistent with the PR's contract.