fix(js): use try_lock in sync setters to avoid event-loop stalls - #41
Open
fornwall wants to merge 1 commit into
Open
fix(js): use try_lock in sync setters to avoid event-loop stalls#41fornwall wants to merge 1 commit into
fornwall wants to merge 1 commit into
Conversation
Synchronous `#[napi]` setters run on the Node.js main (JS) thread, while
async `Task::compute` implementations run on the libuv worker pool. Both
acquire the same per-object mutex (the `Mutex<ManagedConnection>` inside
`AdbcConnectionCore`, and the `Arc<Mutex<CoreStatement>>` of a statement).
Previously these setters used a blocking `lock()`. If a long-running async
operation (e.g. `ExecuteQueryTask::compute`) held the mutex on a worker
thread, a synchronous setter called from JS would block the entire Node
event loop until the async driver call finished.
Switch the synchronous setters to `try_lock()` and surface a clear JS
exception ("connection is busy with another operation" / "statement is
busy with another operation") when the object is currently in use, instead
of blocking. Poisoned-mutex handling keeps parity with the prior behavior.
The async `Task` implementations are unchanged: they legitimately block on
the worker pool, off the JS thread.
Methods changed:
- `AdbcConnectionCore::set_option` (client.rs) — backs
`_NativeAdbcConnection::set_option`
- `_NativeAdbcStatement::set_sql_query` (lib.rs)
- `_NativeAdbcStatement::set_option` (lib.rs)
A dedicated `ClientError::Busy` variant carries the busy message without
the generic "Internal Error:" prefix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNCrC87g9MkppGpL4MDgh5
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.
Problem
Synchronous
#[napi]setters run on the Node.js main (JS) thread, while asyncTask::computeimplementations run on the libuv worker pool. Both acquire the same per-object mutex:Mutex<ManagedConnection>insideAdbcConnectionCore, andArc<Mutex<CoreStatement>>of a statement.These setters previously used a blocking
lock(). When a long-running async operation (e.g.ExecuteQueryTask::compute) holds the mutex on a worker thread, a synchronous setter called from JS blocks the entire Node event loop until the async driver call finishes.Fix
Switch the synchronous setters to
try_lock(). When the object is currently busy (mutex held by an in-flight async op), throw a clear JS exception instead of blocking:"connection is busy with another operation""statement is busy with another operation"Poisoned-mutex handling keeps parity with the prior behavior. The async
Taskimplementations are intentionally left unchanged — they legitimately block on the worker pool, off the JS thread.Methods changed
AdbcConnectionCore::set_option(client.rs) — backs_NativeAdbcConnection::set_option_NativeAdbcStatement::set_sql_query(lib.rs)_NativeAdbcStatement::set_option(lib.rs)A dedicated
ClientError::Busyvariant carries the busy message without the generic"Internal Error:"prefix.Verification
cd javascript && cargo buildsucceeds.🤖 Generated with Claude Code
https://claude.ai/code/session_01XNCrC87g9MkppGpL4MDgh5