W-23692110: Multiple isolated DataWeave engines per process (Node) - #157
Open
mlischetti wants to merge 34 commits into
Open
W-23692110: Multiple isolated DataWeave engines per process (Node)#157mlischetti wants to merge 34 commits into
mlischetti wants to merge 34 commits into
Conversation
Addresses GUS W-23692110, discovered while implementing Node.js external module support (#154). native-lib's ScriptRuntime is a static singleton with a write-once resolver, so a second DataWeave instance in one Node process silently reuses the first instance's resolver instead of getting its own. Design: turn ScriptRuntime into a handle-addressable registry of per-instance engines (one shared GraalVM isolate, following the pattern native-cli's NativeRuntime already uses), with a per-handle resolver bridge in the Node C addon. Python is out of scope here (tracked as a follow-up) since it already gets isolation via one isolate per instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…egression test Rewires ffi.ts and dataweave.ts to call the new handle-based N-API methods (createEngine/createEngineWithResolver/destroyEngine/ runScriptEngine/runScriptStreamingEngine/runScriptTransformEngine) added in Task 3, removing runWithResolver. Each DataWeave instance now owns its own engineHandle, created on initialize() and destroyed on cleanup(), so multiple instances with different resolvers no longer cross-talk in the same process. Adds independent-engines.test.ts proving two resolver-backed instances resolve only their own modules, that a genuine script error on the new handle-based run() path surfaces as success:false rather than an unhandled throw (runScriptEngine now returns "" instead of throwing on a NULL native result), and that runStreaming/runTransform correctly thread the handle through addon.c's argument-shifted N-API wiring. Deletes the now-obsolete first-resolver-wins regression test and fixture, and rewrites dataweave-resolver.test.ts so each test builds its own minimal resolver map instead of sharing a process-wide "first resolver wins" module map.
…itialize() failure If ffi.initialize() succeeded but engine creation (createEngine/ createEngineWithResolver) then threw, this.initialized stayed false, so cleanup()'s early-return guard meant ffi.cleanup() was never called -- permanently leaking that instance's increment of the native library's ref-counted handle. initialize()'s catch block now releases that ref-count itself (ffi.cleanup()) when ffi.initialize() already succeeded, before wrapping and re-throwing. Adds tests/unit/dataweave-initialize.test.ts, a new unit-lane test (mocked ffi module, no dwlib required) exercising this exact sequencing bug plus the surrounding invariants: no cleanup() call when ffi.initialize() itself fails, no residual state after a failed attempt, and no spurious cleanup() call on the successful path.
…ps (F1, F2) Resolver-backed engine bridges could be freed while a background streaming/ transform uv_thread still dereferenced them via resolve_module_callback (F1), and napi_cleanup deleted thread-affine napi_refs from whatever thread made the last release (F2, undefined behavior across Workers). F1: add in_flight/destroy_pending accounting (under g_mutex). Streaming/transform setup pins the bridge via bridge_begin_op before spawning the worker thread; the completion sentinel releases it via bridge_end_op on the owner thread. destroyEngine unlinks immediately but defers the free (napi_ref delete + struct free) to the last draining op when in_flight > 0. F2: register a per-env cleanup hook (napi_add_env_cleanup_hook) per bridge at creation so each Worker/main env disposes its own napi_ref on its own thread; destroyEngine removes the hook before an early free. napi_cleanup no longer touches g_bridges and only performs the process-global GraalVM isolate teardown once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…k (F3, F4) create_engine/create_engine_with_resolver are GraalVM @CEntryPoints; if Java construction throws, the entrypoint returns the long long default value (0) instead of propagating. Treat any handle <= 0 as invalid: throw an N-API error and unwind the bridge (delete napi_ref, free struct) before it's ever linked into g_bridges or given a cleanup hook, instead of returning/inserting a bogus handle. Also fix a resolver-source buffer leak: if the malloc for the tracking node itself fails, the buffer was previously left untracked and unfreeable. resolver_results_track now reports tracking failure so resolve_module_callback can free the buffer and report "unresolved" instead of leaking it.
… path The handle <= 0 rejection path did manual napi_delete_reference + free(bridge) instead of bridge_finalize, so any resolver-callback buffers already tracked via resolver_results_track (if resolve_module_callback ran during a failed eager module setup before construction was reported as failed) were leaked. bridge_finalize already frees tracked buffers before freeing the struct and is a safe drop-in here since the bridge was never linked into g_bridges or given a cleanup hook at this point.
Node addon.c: fail initialize() with a clear message when dwlib lacks the per-engine symbols (create_engine, create_engine_with_resolver, destroy_engine, run_script_engine, run_script_callback_engine, run_script_input_output_callback_engine) instead of deferring to a confusing per-call error, since every initialize() now creates an engine. CallbackWeaveResourceResolver.resolve(): suppress exception detail by default and only log e.getMessage() when DATAWEAVE_RESOLVER_DEBUG=1, matching the C-side resolve_module_callback policy in addon.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…free The default (non-debug) branch of CallbackWeaveResourceResolver.resolve()'s catch block still logged the module path unconditionally, which is dynamic, resolver-controlled content. Drop path too in the default branch so the log line is fully static, matching the C-side resolve_module_callback's actual default behavior in addon.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds four resolver-backed integration tests to dataweave-resolver.test.ts that exercise paths untested by prior remediation commits: - a throwing resolveModule() causes run() to fail cleanly (success:false) rather than crash, exercising resolve_module_callback's exception catch/clear/log-gated-by-DATAWEAVE_RESOLVER_DEBUG path. - a resolver-backed instance's initialize -> cleanup -> initialize cycle still resolves a custom module afterwards (fresh engine_bridge_t). - cleanup() raced against an in-flight resolver-backed runStreaming() does not crash -- the regression test for the F1 in-flight-refcount fix, started deterministically by calling gen.next() without awaiting it before calling cleanup(), so the native call is already handed to the libuv worker thread when cleanup() runs on the JS thread. - run() after cleanup() throws DataWeaveError via dataweave.ts's ensureInitialized() guard (the TS-level half of the destroyed/unknown engine handle contract).
Extracts the "Unknown engine handle" JSON literal shared by run_script_engine, run_script_callback_engine, and run_script_input_output_callback_engine into a single package-visible constant (NativeLib.UNKNOWN_ENGINE_HANDLE_JSON), so the exact error contract can be asserted from a plain JVM unit test. The @centrypoint methods themselves can't be exercised directly from a JVM test since their GraalVM word-type parameters (IsolateThread, CCharPointer) only resolve inside a compiled native image. Adds ScriptRuntimeTest#unknownEngineHandleProducesExactErrorJson, which combines that constant assertion with the existing proof that ScriptRuntime.get() returns null for an unregistered handle.
These were internal review artifacts incidentally committed during remediation work (one references a local temp worktree path); they aren't product documentation and shouldn't ship in the repo.
Contributor
Author
|
Pushed remediation for the two code reviews (
A final whole-branch review across all 14 commits came back clean (no Critical/Important findings); the two Minor findings it raised (native-level test for the "Unknown engine handle" JSON contract, and stray review-notes files) are fixed in the last two commits. |
The follow-up PR-157 review found that DataWeave.cleanup() can deadlock the process when called while a runStreaming()/runTransform() operation is still in flight: isolate teardown blocks the JS thread that a mid-delivery worker's threadsafe-function call depends on. This design makes teardown async and wait for active ops to drain via a dedicated waiter thread, instead of blocking inline.
…completion wakeups Changed uv_cond_signal to uv_cond_broadcast in the op-completion sentinels (call_js_write and call_js_transform_write) to prevent the signal from being stolen by a concurrent initialize() waiter, which would cause a deadlock where teardown_waiter_thread_fn never receives the wakeup it needs to detect g_active_ops reached 0.
…thread, not the JS-thread callback
…n failure If uv_thread_create_ex() fails for the streaming or transform background worker, nothing ever ran to decrement g_active_ops or release the resolver bridge hold, permanently wedging cleanup(). Capture the spawn return value and, on failure, unwind everything committed since the promise was created (g_active_ops decrement, bridge_end_op, threadsafe function release, deferred resolution with an error sentinel, and frees) in the same order as the existing completion branches, minus the thread join. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
napi_cleanup case 5 ignored uv_thread_create_ex's return value when spawning the teardown waiter thread. If the spawn fails, g_teardown_pending would stay true forever, permanently blocking every future initialize() and cleanup() call. Capture the spawn result and, on failure, roll back g_teardown_pending, detach the enqueued waiter, resolve its promise inline, release its threadsafe function, and restore g_ref_count to 1 so the isolate is correctly treated as still live. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
call_js_read early-returned without signaling req->cond when N-API invokes it with env == NULL during environment teardown (e.g. a Worker terminating mid-transform) while data is non-NULL. transform_read_cb blocks synchronously on that same condition variable, so the early return left it hung forever, stranding the worker thread's isolate detach. Restructure to treat env == NULL (with live data) as a terminal read error: set bytes_read = -1 and fall through to the existing signal block, so the blocked waiter always wakes exactly once. The data == NULL branch (nothing to signal) is untouched. Also added confirming comments on call_js_write and call_js_transform_write noting their env == NULL early-returns are not the same bug: their completion path is driven by a separately-enqueued sentinel chunk, not a synchronously-blocked waiter.
Node's exit hook runs synchronously, so an in-flight streaming/transform operation gets abandoned if the process exits normally while cleanup()'s drain hasn't finished. Add a beforeExit handler that awaits cleanup() for the graceful common case, keeping exit as a synchronous last-ditch fallback for process.exit()/signals where beforeExit never fires. A cleanupStarted guard prevents the two hooks from double-driving cleanup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Previously the guard latched true on the first beforeExit and was never reset, so a singleton revived after a beforeExit-driven cleanup would register a new hook pair that could never fire cleanup() at the real exit, silently defeating the graceful-drain guarantee. Resetting the flag as the last step of cleanup() (after the drain finishes) fixes this without affecting the exit handler's own guard check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(F1) Capture the return value of uv_thread_create_ex() at three sites (napi_initialize, napi_cleanup case 4, and dw_napi_run_script) and only call uv_thread_join() if the spawn succeeded. Fixes undefined behavior on thread/resource exhaustion when joining an uninitialized thread handle. Site A (napi_initialize): fail early with explicit error. Site B (napi_cleanup case 4): best-effort degradation, clear global state unconditionally (isolate teardown is a best-effort concern here). Site C (dw_napi_run_script): fail fast with explicit error; same pattern as Site A since runScript has no valid degraded fallback and no deferred result. Free script/inputs buffers on error path to avoid leak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
call_js_write and call_js_transform_write early-returned on env == NULL, skipping all native cleanup (worker thread join, threadsafe function release, bridge_end_op, and every heap free) for the completion sentinel when Node invokes the tsfn callback during env/Worker teardown. This could leak the work struct and strand a bridge marked for deferred destruction indefinitely. Restructure both callbacks so env == NULL still performs full native finalization on the sentinel path (join, tsfn release(s), bridge_end_op, frees), skipping only the napi-value/JS-calling calls (napi_create_string_utf8/napi_resolve_deferred) that require a live env. A non-sentinel data chunk arriving with env == NULL now frees chunk->buf/ chunk instead of leaking them, without touching the work struct. Confirmed via the N-API docs that napi_release_threadsafe_function (whose signature takes no env and is documented as callable from any thread) and uv_thread_join are legal to call during this env == NULL invocation; only JS-calling/napi-value-producing APIs are restricted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
teardown_waiter_thread_fn unconditionally cleared g_thread, g_isolate, g_initialized, and g_ref_count after attempting to attach a waiter thread and tear down the isolate, even if the attach step failed. When attach fails, the underlying isolate is still alive but becomes unreachable through the addon's globals, so a later initialize() would create a second isolate and the original could never be torn down. Track whether teardown actually happened (or whether there was nothing to tear down in the first place) and only clear those four globals in that case. g_teardown_pending still clears unconditionally, since leaving it set would permanently wedge future initialize()/cleanup() calls; on the attach-failure path g_initialized stays 1 and g_isolate stays non-NULL, so napi_initialize's wait guard passes and ref-counts the existing isolate instead of building a second one, and the failed teardown is retried on the next last-release cleanup().
…t strand DataWeave If ffi.cleanup() rejects, the previous code left `initialized` stuck true even though engineHandle was already nulled, permanently short-circuiting a later initialize() via its no-op guard. Wrap the body in try/finally so `initialized` is always cleared, letting the instance be re-initialized after a failed cleanup. Adds a regression test that stubs ffi.cleanup() to reject once, awaits the rejection, then asserts a subsequent initialize() actually calls ffi.initialize()/createEngine() again rather than no-op'ing.
Two Important findings from the final whole-branch review of this remediation round: 1. bridge_finalize deleted a bridge's resolver napi_ref based only on b->env being non-NULL, which stays true even after the owning env dies. The env==NULL sentinel path in call_js_write/ call_js_transform_write can reach bridge_finalize via bridge_end_op while that same env is tearing down, violating N-API's env-liveness contract. Thread an explicit env_still_alive flag through bridge_end_op/bridge_finalize from every call site so the ref deletion is skipped whenever the owning env is known dead; Node auto-reclaims the ref in that case, so nothing leaks. 2. napi_cleanup's case 4 fast path unconditionally cleared g_thread/g_isolate/g_initialized/g_ref_count after spawning cleanup_thread_fn, even though that thread can silently return without tearing down the isolate (attach failure). Mirror the torn_down out-param pattern already used by teardown_waiter_thread_fn: cleanup_thread_fn now reports whether it actually tore down (or had nothing to tear down) via an int* out-param, and case 4 only clears the globals when torn_down is true -- otherwise the isolate stays reachable for a future initialize() instead of being orphaned. Verified: npm run build:addon succeeds; npm test passes 864/59 skipped/0 failed, matching baseline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
native-lib's process-wideScriptRuntimesingleton (one engine, write-once resolver, first-caller-wins) with a handle-keyed registry of per-engineScriptRuntimeobjects living in one shared GraalVM isolate — closing W-23692110 for the Node binding.DataWeaveNode instance now owns an independent native engine (its own module resolver and script cache) addressed by an opaque handle, so multiple instances with different resolvers coexist in one process with no cross-talk.run_script,run_script_callback,run_script_input_output_callback) andScriptRuntime.getInstance()are unchanged, so the Python binding is unaffected. Python's own migration to per-instance engines is a separate follow-up.Design
See
docs/superpowers/specs/2026-08-07-native-lib-multi-engine-design.md(commit 729ed19) for the full design.Test plan
./gradlew native-lib:test --tests "org.mule.weave.lib.ScriptRuntimeTest"— registry isolation, cross-talk, and built-ins-only engines pass; legacygetInstance()tests unaffected../gradlew native-lib:nativeCompile— newcreate_engine*/*_enginesymbols exported, old*_with_resolversymbols removed../gradlew native-lib:nodeTest— 854 passed / 59 skipped, including the newindependent-enginesregression and TCK conformance../gradlew native-lib:pythonTest— 17/17 passed, confirming the legacy Python-facing surface is untouched.🤖 Generated with Claude Code