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
18 changes: 18 additions & 0 deletions src/runtime/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,24 @@ pub(crate) fn __bun_release_task_at_shutdown(task: bun_event_loop::Task) -> bool
unsafe { Bun__deleteDeferredWorkTask(task.ptr.cast::<JSCDeferredWorkTask>()) };
true
}
// A libc `getaddrinfo` completion the work pool posted back but
// `tick()` never dispatched. Free the `WorkTask`, the
// `GetAddrInfoRequest`, and the `DNSLookup` chain so their
// `JSPromiseStrong` handles drop while the JSC VM is still live;
// re-queuing would strand them past worker VM dealloc.
#[cfg(not(windows))]
task_tag::GetAddrInfoRequestTask => {
let wt = task.ptr.cast::<get_addr_info_request::Task>();
// SAFETY: tag identifies pointee; `ctx` is the heap request
// `WorkTask::create_on_js_thread` stored.
let ctx = unsafe { (*wt).ctx };
// SAFETY: `ctx` was `heap::into_raw`'d in `GetAddrInfoRequest::init`
// and the work-pool `run` has completed (it posted this entry).
unsafe { crate::dns_jsc::GetAddrInfoRequest::release_for_shutdown(ctx) };
// SAFETY: paired with `create_on_js_thread` heap::alloc.
unsafe { get_addr_info_request::Task::destroy(wt) };
true
}
// Same reclaim `drop_concurrent_cpp_tasks` performs, but for tasks
// that were already batch-moved into `self.tasks`. Must run before
// JSC teardown: a Worker `dispatchExit` lambda's `~Ref<Worker>` walks
Expand Down
64 changes: 58 additions & 6 deletions src/runtime/dns_jsc/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,12 @@ impl CAresNameInfo {
let mut promise = unsafe { core::mem::take(&mut (*this).promise) };
// SAFETY: see fn contract — `this` is a live node.
let global_this = unsafe { (*this).global_this() };
let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards
// An empty `result` means the caller's JS-result conversion threw
// (JSC termination during worker teardown). Resolving with it would
// trip `ASSERT(!target.isEmpty())` in `JSC__JSPromise__resolve`.
if !result.is_empty() {
let _ = promise.resolve_task(global_this, result);
}
// SAFETY: see fn contract.
unsafe { Self::destroy(this) };
}
Expand Down Expand Up @@ -1561,6 +1566,37 @@ impl GetAddrInfoRequest {
}
}

/// Shutdown-time reclaim for a queued-but-never-dispatched completion
/// (`__bun_release_task_at_shutdown`'s `GetAddrInfoRequestTask` arm).
/// Frees the request box and every `DNSLookup` in its chain so their
/// `JSPromiseStrong` handles drop while the JSC VM is still live; does not
/// run JS. The resolver's pending-native-cache slot (if any) is popped so
/// nothing is left pointing at the freed request.
///
/// # Safety
/// `this` must be the live heap `GetAddrInfoRequest` whose `run` already
/// completed; consumed (freed) here.
#[cfg(not(windows))]
pub unsafe fn release_for_shutdown(this: *mut Self) {
// SAFETY: per fn contract — `this` is the live heap request.
unsafe {
if let Some(resolver) = (*this).resolver_for_caching {
if (*this).cache.pending_cache() {
let _ = (*resolver).get_key_host(
(*this).cache.pos_in_pending(),
PendingCacheField::PendingHostCacheNative,
);
}
}
let mut next = (*this).head.next.take();
while let Some(n) = next {
next = (*n.as_ptr()).next.take();
DNSLookup::destroy(n.as_ptr());
}
drop(bun_core::heap::take(this));
}
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
#[cfg(windows)]
pub fn on_libuv_complete(uv_info: *mut libuv::uv_getaddrinfo_t) {
unsafe {
Expand Down Expand Up @@ -1731,7 +1767,12 @@ impl CAresReverse {
unsafe {
let mut promise = core::mem::take(&mut (*this).promise);
let global_this = (*this).global_this();
let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards
// An empty `result` means the caller's JS-result conversion threw
// (JSC termination during worker teardown). Resolving with it would
// trip `ASSERT(!target.isEmpty())` in `JSC__JSPromise__resolve`.
if !result.is_empty() {
let _ = promise.resolve_task(global_this, result);
}
if let Some(resolver) = (*this).resolver.as_ref() {
// IntrusiveRc holds a live ref; request_completed mutates pending_requests counter only.
(*resolver.as_ptr()).request_completed();
Expand Down Expand Up @@ -1881,7 +1922,12 @@ impl<T: CAresRecordType> CAresLookup<T> {
unsafe {
let mut promise = core::mem::take(&mut (*this).promise);
let global_this = (*this).global_this();
let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards
// An empty `result` means the caller's JS-result conversion threw
// (JSC termination during worker teardown). Resolving with it would
// trip `ASSERT(!target.isEmpty())` in `JSC__JSPromise__resolve`.
if !result.is_empty() {
let _ = promise.resolve_task(global_this, result);
}
if let Some(resolver) = (*this).resolver.as_ref() {
// IntrusiveRc holds a live ref; request_completed mutates pending_requests counter only.
(*resolver.as_ptr()).request_completed();
Expand Down Expand Up @@ -2059,7 +2105,12 @@ impl DNSLookup {
unsafe {
let mut promise = core::mem::take(&mut (*this).promise);
let global_this = (*this).global_this();
let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards
// An empty `result` means the caller's JS-result conversion threw
// (JSC termination during worker teardown). Resolving with it would
// trip `ASSERT(!target.isEmpty())` in `JSC__JSPromise__resolve`.
if !result.is_empty() {
let _ = promise.resolve_task(global_this, result);
}
if let Some(resolver) = (*this).resolver.as_ref() {
// IntrusiveRc holds a live ref; request_completed mutates pending_requests counter only.
(*resolver.as_ptr()).request_completed();
Expand Down Expand Up @@ -4526,8 +4577,9 @@ impl Resolver {
pending = (*value.as_ptr()).next;
if !core::ptr::eq(prev_global, new_global) {
array = super::options_jsc::result_any_to_js(result, new_global)
.unwrap_or(None)
.unwrap(); // TODO: properly propagate exception upwards
.ok()
.flatten()
.unwrap_or(JSValue::ZERO);
prev_global = new_global;
}

Expand Down
65 changes: 64 additions & 1 deletion test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, isDebug } from "harness";
import { bunEnv, bunExe, isASAN, isDebug, isWindows } from "harness";
import { join } from "path";

// Worker VM startup/teardown is much slower under debug and/or ASAN; these
Expand Down Expand Up @@ -176,3 +176,66 @@ test.skipIf(!isASAN)(
},
timeout,
);

// Regression: a dns.lookup() completion dispatched on the worker thread while
// the parent's terminate() has already set the JSC termination flag would see
// the result-to-JSArray conversion throw, leaving the value as JSValue::ZERO,
// and then pass that empty value straight to JSC__JSPromise__resolve, hitting
// ASSERT(!target.isEmpty()) in debug and a near-null (0x5) SIGSEGV in release.
//
// The worker queues 128 distinct IP-literal lookups with backend:"getaddrinfo"
// so each becomes its own GetAddrInfoRequest on the work pool (the
// pending-cache hive is 32 slots, so most bypass it), blocks in Atomics.wait
// until those completions have piled up in concurrent_tasks, then the parent
// wakes it and terminates it while the drain is running. Hermetic: IP literals
// only. Windows uses uv_getaddrinfo which does not dispatch via
// GetAddrInfoRequestTask, so the reproduction path does not apply there.
test.skipIf(isWindows)(
"terminate() while dns.lookup() completions are draining does not resolve with an empty value",
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const { Worker } = require("node:worker_threads");
const src =
'const { parentPort, workerData } = require("node:worker_threads");' +
'const ia = new Int32Array(workerData);' +
'setImmediate(() => {' +
' for (let i = 1; i <= 128; i++)' +
' Bun.dns.lookup("127.0.0." + i, { backend: "getaddrinfo" }).catch(() => {});' +
' parentPort.postMessage("up");' +
' Atomics.wait(ia, 0, 0);' +
'});' +
'setTimeout(() => {}, 100000);';
async function once() {
const sab = new SharedArrayBuffer(4);
const ia = new Int32Array(sab);
const w = new Worker(src, { eval: true, workerData: sab });
await new Promise((res) => w.once("message", res));
await Bun.sleep(40);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Atomics.store(ia, 0, 1);
Atomics.notify(ia, 0);
await w.terminate();
}
(async () => {
for (let r = 0; r < ${slow ? 6 : 10}; r++) {
await Promise.all([once(), once(), once(), once()]);
}
console.log("ok");
})();
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout).toBe("ok\n");
expect(exitCode).toBe(0);
},
timeout,
);
Loading