Skip to content

Commit c48ac5c

Browse files
simongdaviesCopilot
andcommitted
refactor: address PR review feedback
- Make setHostPrintFn callback synchronous (remove Promise deferral) - Add TypeError validation for non-function callback arguments - Switch host function TSFN calls to Blocking mode for consistency - Remove microtask flush workarounds from tests - Update reentrancy documentation comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6d4c32a commit c48ac5c

3 files changed

Lines changed: 21 additions & 29 deletions

File tree

src/js-host-api/lib.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -218,16 +218,16 @@ for (const method of [
218218
}
219219
SandboxBuilder.prototype.setHostPrintFn = wrapSync(function (callback) {
220220
if (typeof callback !== 'function') {
221-
// Forward non-function values to the native method for consistent
222-
// validation errors (the Rust layer rejects non-callable arguments).
223-
return origSetHostPrintFn.call(this, callback);
221+
throw new TypeError(
222+
`SandboxBuilder.setHostPrintFn expects a function, received ${typeof callback}`
223+
);
224224
}
225225
return origSetHostPrintFn.call(this, (msg) => {
226-
Promise.resolve()
227-
.then(() => callback(msg))
228-
.catch((e) => {
229-
console.error('Host print callback threw:', e);
230-
});
226+
try {
227+
callback(msg);
228+
} catch (e) {
229+
console.error('Host print callback threw:', e);
230+
}
231231
});
232232
});
233233
}

src/js-host-api/src/lib.rs

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -426,20 +426,18 @@ impl SandboxBuilderWrapper {
426426
>,
427427
) -> napi::Result<&Self> {
428428
self.with_inner(|b| {
429-
// Blocking mode ensures the TSFN dispatch completes before the
430-
// native call returns, but the JS wrapper defers the user callback
431-
// via a Promise microtask — so user code may run after guest
432-
// execution resumes. From the guest's perspective, print is
433-
// effectively fire-and-forget with no return value to await.
434-
// Unlike host functions (which use NonBlocking + oneshot channel
435-
// for async Promise resolution), print has no result path.
429+
// Blocking mode ensures the TSFN call is queued even when the
430+
// queue is full (it blocks until space is available), preventing
431+
// silent message drops that NonBlocking mode would cause.
436432
//
437-
// **Reentrancy note:** The print callback ultimately runs while
438-
// the sandbox Mutex is held (inside `call_handler`'s
439-
// `spawn_blocking`). Calling Hyperlight APIs that acquire the
440-
// same lock from within the callback (e.g. `snapshot()`,
441-
// `restore()`, `unload()`) will deadlock. Keep print callbacks
442-
// simple — logging only.
433+
// The JS wrapper invokes the user callback synchronously in the
434+
// TSFN handler — no microtask deferral.
435+
//
436+
// **Reentrancy note:** The print callback runs while the sandbox
437+
// Mutex is held (inside `call_handler`'s `spawn_blocking`).
438+
// Calling Hyperlight APIs that acquire the same lock from within
439+
// the callback (e.g. `snapshot()`, `restore()`, `unload()`) will
440+
// deadlock. Keep print callbacks simple — logging only.
443441
let print_fn = move |msg: String| -> i32 {
444442
let status = callback.call(msg, ThreadsafeFunctionCallMode::Blocking);
445443
if status == Status::Ok {
@@ -685,10 +683,10 @@ impl HostModuleWrapper {
685683
return Err(invalid_arg_error("Function name must not be empty"));
686684
}
687685
let wrapper = move |args: String| -> hyperlight_js::Result<String> {
688-
use ThreadsafeFunctionCallMode::NonBlocking;
686+
use ThreadsafeFunctionCallMode::Blocking;
689687
let args: Vec<Option<serde_json::Value>> = serde_json::from_str(&args)?;
690688
let (tx, rx) = oneshot::channel();
691-
let status = func.call_with_return_value(Rest(args), NonBlocking, move |result, _| {
689+
let status = func.call_with_return_value(Rest(args), Blocking, move |result, _| {
692690
let _ = tx.send(result);
693691
Ok(())
694692
});

src/js-host-api/tests/sandbox.test.js

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -471,8 +471,6 @@ describe('setHostPrintFn', () => {
471471
);
472472
const loaded = await sandbox.getLoadedSandbox();
473473
await loaded.callHandler('handler', {});
474-
// Flush microtasks — the print wrapper defers via Promise
475-
await new Promise((r) => setTimeout(r, 0));
476474

477475
expect(messages.join('')).toContain('Hello from guest!');
478476
});
@@ -495,8 +493,6 @@ describe('setHostPrintFn', () => {
495493
);
496494
const loaded = await sandbox.getLoadedSandbox();
497495
await loaded.callHandler('handler', {});
498-
// Flush microtasks
499-
await new Promise((r) => setTimeout(r, 0));
500496

501497
const combined = messages.join('');
502498
expect(combined).toContain('first');
@@ -545,8 +541,6 @@ describe('setHostPrintFn', () => {
545541
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
546542
try {
547543
const result = await loaded.callHandler('handler', {});
548-
// Flush microtasks — the print wrapper defers via Promise
549-
await new Promise((r) => setTimeout(r, 0));
550544

551545
// The JS wrapper catches the throw — guest continues normally
552546
expect(result.survived).toBe(true);

0 commit comments

Comments
 (0)