Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/js-host-api/lib.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,30 @@ for (const method of [
SandboxBuilder.prototype[method] = wrapSync(orig);
}

// setHostPrintFn needs a custom wrapper: the user's callback is wrapped in
// try/catch before it reaches the native layer, because exceptions thrown
// inside a Blocking ThreadsafeFunction escape as unhandled errors.
{
const origSetHostPrintFn = SandboxBuilder.prototype.setHostPrintFn;
Comment thread
simongdavies marked this conversation as resolved.
if (!origSetHostPrintFn) {
throw new Error('Cannot wrap missing method: SandboxBuilder.setHostPrintFn');
}
SandboxBuilder.prototype.setHostPrintFn = wrapSync(function (callback) {
if (typeof callback !== 'function') {
throw new TypeError(
`SandboxBuilder.setHostPrintFn expects a function, received ${typeof callback}`
);
}
return origSetHostPrintFn.call(this, (msg) => {
try {
callback(msg);
} catch (e) {
console.error('Host print callback threw:', e);
}
});
});
Comment thread
simongdavies marked this conversation as resolved.
}

// ── Re-export ────────────────────────────────────────────────────────

module.exports = native;
126 changes: 113 additions & 13 deletions src/js-host-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ enum ErrorCode {
Consumed,
/// Internal / unexpected failure (lock poison, task join error, etc.).
Internal,
/// Reentrant call detected — calling sandbox methods from a host callback.
Reentrant,
}

impl ErrorCode {
Expand All @@ -135,6 +137,7 @@ impl ErrorCode {
Self::InvalidArg => "ERR_INVALID_ARG",
Self::Consumed => "ERR_CONSUMED",
Self::Internal => "ERR_INTERNAL",
Self::Reentrant => "ERR_REENTRANT",
}
}
}
Expand Down Expand Up @@ -230,6 +233,15 @@ fn join_error(err: tokio::task::JoinError) -> napi::Error {
)
}

/// Creates an error for reentrant calls detected at runtime.
fn reentrant_error() -> napi::Error {
hl_error(
ErrorCode::Reentrant,
"Cannot call sandbox methods from a host callback — the sandbox lock is held during \
guest execution. Perform this operation after callHandler() resolves instead",
)
}

// ── Snapshot ─────────────────────────────────────────────────────────

/// A captured point-in-time state of a sandbox.
Expand Down Expand Up @@ -401,6 +413,55 @@ impl SandboxBuilderWrapper {
inner: Arc::new(Mutex::new(Some(proto_sandbox))),
})
}

/// Set a callback that receives guest `console.log` / `print` output.
///
/// Without this, guest print output is silently discarded. The callback
/// receives each print message as a string.
///
/// If the callback throws, the exception is caught by the JS wrapper
/// (`lib.js`) and logged to `console.error`. Guest execution continues.
///
/// @param callback - `(message: string) => void` — called for each print
/// @returns this (for chaining)
/// @throws If the builder has already been consumed by `build()`
#[napi]
pub fn set_host_print_fn(
&self,
#[napi(ts_arg_type = "(message: string) => void")] callback: ThreadsafeFunction<
String, // Rust → JS argument type
(), // JS return type (void)
String, // JS → Rust argument type (same — identity mapping)
Status, // Error status type
false, // Not CallerHandled (napi manages errors)
false, // Not accepting unknown return types
>,
) -> napi::Result<&Self> {
self.with_inner(|b| {
// Blocking mode ensures the TSFN call is queued even when the
// queue is full (it blocks until space is available), preventing
// silent message drops that NonBlocking mode would cause.
//
// The JS wrapper invokes the user callback synchronously in the
// TSFN handler — no microtask deferral.
//
// **Reentrancy note:** The print callback runs while the sandbox
// Mutex is held (inside `call_handler`'s `spawn_blocking`).
// If user code in the callback attempts to call methods that
// acquire the same lock (e.g. `snapshot()`, `restore()`,
// `unload()`, `callHandler()`), the `executing_flag` deadlock
// detection will return `ERR_REENTRANT` instead of hanging.
let print_fn = move |msg: String| -> i32 {
let status = callback.call(msg, ThreadsafeFunctionCallMode::Blocking);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, reading the nodejs documentation
https://nodejs.org/api/n-api.html#napi-threadsafe-function-call-mode

A value to be given to napi_call_threadsafe_function() to indicate whether the call should block whenever the queue associated with the thread-safe function is full.

IIUC, this is not blocking for the function to finish executing, but for the call to be queued, I don't know what happens in non-blocking mode. I think we should use blocking mode in the host function calls as well (and wait for the result).

From here https://nodejs.org/api/n-api.html#calling-a-thread-safe-function

napi_call_threadsafe_function() accepts a parameter which controls whether the API behaves blockingly. If set to napi_tsfn_nonblocking, the API behaves non-blockingly, returning napi_queue_full if the queue was full, preventing data from being successfully added to the queue. If set to napi_tsfn_blocking, the API blocks until space becomes available in the queue. napi_call_threadsafe_function() never blocks if the thread-safe function was created with a maximum queue size of 0.

So I think in the host function we are using non-blocking, but we are not checking for the napi_queue_full status.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed in c48ac5c host function TSFN calls now use Blocking mode too (consistent with print). This ensures calls are always queued rather than silently dropped when the queue is full.

Regarding the broader reentrancy concern you raised: we've added runtime deadlock detection in d2dc25a. If a host callback tries to call back into the sandbox while guest code is executing, it returns ERR_REENTRANT instead of hanging. Filed #192 to track properly separating host function dispatch from the sandbox lock (like core hyperlight does with its separate FunctionRegistry mutex).

if status == Status::Ok {
0
} else {
-1
}
};
b.with_host_print_fn(print_fn.into())
})
}
}

// ── ProtoJSSandbox ───────────────────────────────────────────────────
Expand Down Expand Up @@ -635,10 +696,10 @@ impl HostModuleWrapper {
return Err(invalid_arg_error("Function name must not be empty"));
}
let wrapper = move |args: String| -> hyperlight_js::Result<String> {
use ThreadsafeFunctionCallMode::NonBlocking;
use ThreadsafeFunctionCallMode::Blocking;
let args: Vec<Option<serde_json::Value>> = serde_json::from_str(&args)?;
let (tx, rx) = oneshot::channel();
let status = func.call_with_return_value(Rest(args), NonBlocking, move |result, _| {
let status = func.call_with_return_value(Rest(args), Blocking, move |result, _| {
let _ = tx.send(result);
Ok(())
});
Expand Down Expand Up @@ -805,6 +866,7 @@ impl JSSandboxWrapper {
poisoned_flag,
last_call_stats: Arc::new(ArcSwapOption::empty()),
disposed_flag: Arc::new(AtomicBool::new(false)),
executing_flag: Arc::new(AtomicBool::new(false)),
})
}

Expand Down Expand Up @@ -887,17 +949,37 @@ pub struct LoadedJSSandboxWrapper {
/// `unload()`), for lock-free checks in sync getters that don't
/// consult the inner Mutex.
disposed_flag: Arc<AtomicBool>,

/// Tracks whether guest code is currently executing inside `call_handler`.
///
/// Used for deadlock detection: if a host callback (print fn, host function)
/// tries to call a method that needs the inner Mutex while this is `true`,
/// we return `ERR_REENTRANT` immediately instead of deadlocking.
executing_flag: Arc<AtomicBool>,
}

type LoadedJSSandboxGuard = OwnedMappedMutexGuard<Option<LoadedJSSandbox>, LoadedJSSandbox>;

impl LoadedJSSandboxWrapper {
/// Borrow the inner value mutably via Mutex, or error if consumed.
///
/// Performs deadlock detection: if `executing_flag` is set (meaning we're
/// inside a `call_handler` on a background thread), `try_lock` is attempted
/// first. If it fails, we know this is a reentrant call from a host callback
/// and return `ERR_REENTRANT` immediately instead of deadlocking.
async fn with_inner<R>(
&self,
f: impl AsyncFnOnce(LoadedJSSandboxGuard) -> napi::Result<R>,
) -> napi::Result<R> {
let sandbox = self.inner.clone().lock_owned().await;
let sandbox = if self.executing_flag.load(Ordering::Acquire) {
// We're inside a host callback — try_lock to detect reentrancy.
match self.inner.clone().try_lock_owned() {
Ok(guard) => guard,
Err(_) => return Err(reentrant_error()),
}
} else {
self.inner.clone().lock_owned().await
};
let sandbox = OwnedMutexGuard::try_map(sandbox, Option::as_mut)
.map_err(|_| consumed_error("LoadedJSSandbox"))?;
f(sandbox).await
Expand All @@ -906,30 +988,42 @@ impl LoadedJSSandboxWrapper {
/// Borrow the inner value mutably via Mutex, or error if consumed.
/// The closure `f` will run using spawn_blocking, so it can perform long-running operations without
/// blocking the Node.js event loop. This is the main way to interact with the inner `LoadedJSSandbox`.
///
/// Sets `executing_flag` for the duration of the blocking closure so that
/// reentrant calls from host callbacks are detected instead of deadlocking.
async fn with_blocking_inner<R: Send + 'static>(
&self,
f: impl FnOnce(LoadedJSSandboxGuard) -> napi::Result<R> + Send + 'static,
) -> napi::Result<R> {
let executing_flag = self.executing_flag.clone();
self.with_inner(async move |sandbox| {
tokio::task::spawn_blocking(move || f(sandbox))
executing_flag.store(true, Ordering::Release);
let result = tokio::task::spawn_blocking(move || f(sandbox))
.await
.map_err(join_error)?
.map_err(join_error)?;
executing_flag.store(false, Ordering::Release);
result
})
.await
}

/// Take ownership of the inner value, returning a consumed-state error if
/// this instance has already been used.
///
/// Performs the same deadlock detection as `with_inner`.
async fn take_inner_with<R>(
&self,
f: impl AsyncFnOnce(LoadedJSSandbox) -> napi::Result<R>,
) -> napi::Result<R> {
let sandbox = self
.inner
.lock()
.await
.take()
.ok_or_else(|| consumed_error("LoadedJSSandbox"))?;
let sandbox = if self.executing_flag.load(Ordering::Acquire) {
match self.inner.try_lock() {
Ok(mut guard) => guard.take(),
Err(_) => return Err(reentrant_error()),
}
} else {
self.inner.lock().await.take()
}
.ok_or_else(|| consumed_error("LoadedJSSandbox"))?;
self.disposed_flag.store(true, Ordering::Release);
f(sandbox).await
}
Expand All @@ -938,14 +1032,20 @@ impl LoadedJSSandboxWrapper {
/// this instance has already been used.
/// The closure `f` will run using spawn_blocking, so it can perform long-running operations without
/// blocking the Node.js event loop. This is the main way to interact with the inner `LoadedJSSandbox`.
///
/// Sets `executing_flag` for the duration of the blocking closure.
async fn take_blocking_inner_with<R: Send + 'static>(
&self,
f: impl FnOnce(LoadedJSSandbox) -> napi::Result<R> + Send + 'static,
) -> napi::Result<R> {
let executing_flag = self.executing_flag.clone();
self.take_inner_with(async move |sandbox| {
tokio::task::spawn_blocking(move || f(sandbox))
executing_flag.store(true, Ordering::Release);
let result = tokio::task::spawn_blocking(move || f(sandbox))
.await
.map_err(join_error)?
.map_err(join_error)?;
executing_flag.store(false, Ordering::Release);
result
})
.await
}
Expand Down
113 changes: 112 additions & 1 deletion src/js-host-api/tests/sandbox.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Basic sandbox functionality tests
import { describe, it, expect, beforeEach } from 'vitest';
import { describe, it, expect, beforeEach, vi } from 'vitest';
import { SandboxBuilder } from '../lib.js';
import { expectThrowsWithCode, expectRejectsWithCode } from './test-helpers.js';

Expand Down Expand Up @@ -445,3 +445,114 @@ describe('LoadedJSSandbox.unload()', () => {
expectThrowsWithCode(() => loaded.lastCallStats, 'ERR_CONSUMED');
});
});

// ── Host print function ──────────────────────────────────────────────

describe('setHostPrintFn', () => {
it('should support method chaining', () => {
const builder = new SandboxBuilder();
const returned = builder.setHostPrintFn(() => {});
expect(returned).toBe(builder);
});

it('should receive console.log output from the guest', async () => {
const messages = [];
const builder = new SandboxBuilder().setHostPrintFn((msg) => {
messages.push(msg);
});
const proto = await builder.build();
const sandbox = await proto.loadRuntime();
sandbox.addHandler(
'handler',
`function handler(event) {
console.log("Hello from guest!");
return event;
}`
);
const loaded = await sandbox.getLoadedSandbox();
await loaded.callHandler('handler', {});

expect(messages.join('')).toContain('Hello from guest!');
});
Comment thread
simongdavies marked this conversation as resolved.

it('should receive multiple console.log calls', async () => {
const messages = [];
const builder = new SandboxBuilder().setHostPrintFn((msg) => {
messages.push(msg);
});
const proto = await builder.build();
const sandbox = await proto.loadRuntime();
sandbox.addHandler(
'handler',
`function handler(event) {
console.log("first");
console.log("second");
console.log("third");
return event;
}`
);
const loaded = await sandbox.getLoadedSandbox();
await loaded.callHandler('handler', {});

const combined = messages.join('');
expect(combined).toContain('first');
expect(combined).toContain('second');
expect(combined).toContain('third');
});

it('should use the last callback when set multiple times', async () => {
const firstMessages = [];
const secondMessages = [];
const builder = new SandboxBuilder()
.setHostPrintFn((msg) => firstMessages.push(msg))
.setHostPrintFn((msg) => secondMessages.push(msg));
const proto = await builder.build();
const sandbox = await proto.loadRuntime();
sandbox.addHandler(
'handler',
`function handler(event) {
console.log("which callback?");
return event;
}`
);
const loaded = await sandbox.getLoadedSandbox();
await loaded.callHandler('handler', {});

expect(firstMessages.length).toBe(0);
expect(secondMessages.join('')).toContain('which callback?');
});
Comment thread
simongdavies marked this conversation as resolved.

it('should continue guest execution when callback throws', async () => {
const builder = new SandboxBuilder().setHostPrintFn(() => {
throw new Error('print callback exploded');
});
const proto = await builder.build();
const sandbox = await proto.loadRuntime();
sandbox.addHandler(
'handler',
`function handler(event) {
console.log("this will throw in the callback");
return { survived: true };
}`
);
const loaded = await sandbox.getLoadedSandbox();

// Spy on console.error to suppress noise and verify the error is logged
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
try {
const result = await loaded.callHandler('handler', {});

// The JS wrapper catches the throw — guest continues normally
expect(result.survived).toBe(true);
expect(errorSpy).toHaveBeenCalledWith('Host print callback threw:', expect.any(Error));
} finally {
errorSpy.mockRestore();
}
});
Comment thread
simongdavies marked this conversation as resolved.

it('should throw CONSUMED after build()', async () => {
const builder = new SandboxBuilder();
await builder.build();
expectThrowsWithCode(() => builder.setHostPrintFn(() => {}), 'ERR_CONSUMED');
});
});
Loading