Skip to content

dns: keep pending exceptions out of coalesced lookup promise resolves #37004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 3 commits into from
Closed

dns: keep pending exceptions out of coalesced lookup promise resolves #37004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1db68ca
dns: keep pending exceptions out of coalesced lookup promise resolves
robobun Aug 6, 2026
9ad5260
[autofix.ci] apply automated fixes
autofix-ci[bot] Aug 6, 2026
c9125ce
dns: pass the conversion result to the settle helper as a JsResult
robobun Aug 6, 2026
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
195 changes: 105 additions & 90 deletions src/runtime/dns_jsc/dns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,29 @@ impl c_ares::HostentHandler for GetHostByAddrInfoRequest {
}
}

/// Resolve a DNS request promise with the converted result, or reject it with
/// the conversion error. An exception left pending by the settle is reported
/// as unhandled so the drain loops never carry it into the next settle.
Comment on lines +653 to +655

@github-actions github-actions Bot Aug 6, 2026

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

fn settle_lookup_promise(
promise: &mut JSPromiseStrong,
global_this: &JSGlobalObject,
result: JsResult<JSValue>,
) {
let settled = match result {
Ok(value) => promise.resolve_task(global_this, value),
Err(err) => promise.swap().reject(global_this, Err(err)),
};
if settled.is_err() {
global_this.report_active_exception_as_unhandled(jsc::JsError::Thrown);
}
}

fn ensure_result_still_alive(result: JsResult<JSValue>) {
if let Ok(value) = result {
value.ensure_still_alive();
}
}

// ──────────────────────────────────────────────────────────────────────────
// CAresNameInfo
// ──────────────────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -725,19 +748,18 @@ impl CAresNameInfo {
}
return;
};
let array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, global_this)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
let array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, global_this);
// SAFETY: see fn contract.
unsafe { Self::on_complete(this, array) };
}

/// SAFETY: see `process_resolve`.
unsafe fn on_complete(this: *mut Self, result: JSValue) {
unsafe fn on_complete(this: *mut Self, result: JsResult<JSValue>) {
// SAFETY: see fn contract — `this` is a live node.
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
settle_lookup_promise(&mut promise, global_this, result);
// SAFETY: see fn contract.
unsafe { Self::destroy(this) };
}
Expand Down Expand Up @@ -1504,19 +1526,18 @@ impl CAresReverse {
return;
};
// node is a valid c-ares hostent for the callback's duration
let array = super::cares_jsc::hostent_to_js_response(&mut *node, global_this, b"")
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
let array = super::cares_jsc::hostent_to_js_response(&mut *node, global_this, b"");
Self::on_complete(this, array);
}
}

/// SAFETY: see `process_resolve`.
unsafe fn on_complete(this: *mut Self, result: JSValue) {
unsafe fn on_complete(this: *mut Self, result: JsResult<JSValue>) {
// SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request.
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
settle_lookup_promise(&mut promise, 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 @@ -1653,20 +1674,18 @@ impl<T: CAresRecordType> CAresLookup<T> {
};

// node is a valid c-ares reply for the callback's duration; freed by `_free` guard.
let array = (*node)
.to_js_response(global_this, T::TYPE_NAME)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
let array = (*node).to_js_response(global_this, T::TYPE_NAME);
Self::on_complete(this, array);
}
}

/// SAFETY: see `process_resolve`.
unsafe fn on_complete(this: *mut Self, result: JSValue) {
unsafe fn on_complete(this: *mut Self, result: JsResult<JSValue>) {
// SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request.
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
settle_lookup_promise(&mut promise, 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 @@ -1753,10 +1772,19 @@ impl DNSLookup {
bun_output::scoped_log!(DNSLookup, "onCompleteNative");
// SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request.
unsafe {
let array = super::options_jsc::result_any_to_js(result, (*this).global_this())
.ok()
.flatten()
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
let Some(array) =
super::options_jsc::result_any_to_js(result, (*this).global_this()).transpose()
else {
error_to_deferred(
c_ares::Error::ENOTFOUND,
b"getaddrinfo",
None,
&mut (*this).promise,
)
.reject_later((*this).global_this());
Self::destroy(this);
return;
};
Self::on_complete_with_array(this, array);
}
}
Expand Down Expand Up @@ -1827,20 +1855,19 @@ impl DNSLookup {
// owned by the caller's scopeguard; JSGlobalObject outlives the request.
unsafe {
let array =
super::cares_jsc::addr_info_to_js_array(&mut *result, (*this).global_this())
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
super::cares_jsc::addr_info_to_js_array(&mut *result, (*this).global_this());
Self::on_complete_with_array(this, array);
}
}

/// SAFETY: see `on_complete_native`.
unsafe fn on_complete_with_array(this: *mut Self, result: JSValue) {
unsafe fn on_complete_with_array(this: *mut Self, result: JsResult<JSValue>) {
bun_output::scoped_log!(DNSLookup, "onCompleteWithArray");
// SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request.
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
settle_lookup_promise(&mut promise, 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 @@ -4185,30 +4212,27 @@ impl Resolver {
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();
let mut array = (*addr)
.to_js_response(prev_global, T::TYPE_NAME)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
let mut array = (*addr).to_js_response(prev_global, T::TYPE_NAME);
// SAFETY: addr is the c-ares-allocated reply; freed once after all consumers run.
let _free_addr = scopeguard::guard(addr, |a| T::destroy(a));
array.ensure_still_alive();
ensure_result_still_alive(array);
CAresLookup::<T>::on_complete(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));

array.ensure_still_alive();
ensure_result_still_alive(array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = (*addr)
.to_js_response(new_global, T::TYPE_NAME)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
// The settle consumed an Err's pending exception; never reuse it.
if array.is_err() || !core::ptr::eq(prev_global, new_global) {
array = (*addr).to_js_response(new_global, T::TYPE_NAME);
prev_global = new_global;
}
pending = (*value.as_ptr()).next;

array.ensure_still_alive();
ensure_result_still_alive(array);
CAresLookup::<T>::on_complete(value.as_ptr(), array);
array.ensure_still_alive();
ensure_result_still_alive(array);
}
}
}
Expand Down Expand Up @@ -4251,29 +4275,28 @@ impl Resolver {
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();
let mut array = super::cares_jsc::addr_info_to_js_array(&mut *addr, prev_global)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
let mut array = super::cares_jsc::addr_info_to_js_array(&mut *addr, prev_global);
// SAFETY: addr is the c-ares-allocated AddrInfo; freed once after all consumers run.
// Move the raw pointer into the guard so the loop body can keep borrowing `*addr`.
let _free_addr = scopeguard::guard(addr, |a| c_ares::AddrInfo::destroy(a));
array.ensure_still_alive();
ensure_result_still_alive(array);
DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));

array.ensure_still_alive();
ensure_result_still_alive(array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = super::cares_jsc::addr_info_to_js_array(&mut *addr, new_global)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
// The settle consumed an Err's pending exception; never reuse it.
if array.is_err() || !core::ptr::eq(prev_global, new_global) {
array = super::cares_jsc::addr_info_to_js_array(&mut *addr, new_global);
prev_global = new_global;
}
pending = (*value.as_ptr()).next;

array.ensure_still_alive();
ensure_result_still_alive(array);
DNSLookup::on_complete_with_array(value.as_ptr(), array);
array.ensure_still_alive();
ensure_result_still_alive(array);
}
}
}
Expand All @@ -4291,33 +4314,25 @@ impl Resolver {
// SAFETY: `self` is the live heap allocation; ref_scope keeps count > 0 across re-entrant callbacks.
let _g = unsafe { Self::ref_scope(self.as_ctx_ptr()) };

let mut array: JSValue = match super::options_jsc::result_any_to_js(result, global_object)
.unwrap_or(None)
{
// TODO: properly propagate exception upwards
Some(a) => a,
None => {
// SAFETY: `key.lookup` is the heap-allocated request stored in the
// pending-cache slot; consumed via `heap::take` below.
unsafe {
let mut pending = (*key.lookup).head.next;
// Consume the request and move `head` out by value;
// `ptr::read` + `heap::take` would double-Drop `DNSLookup`.
let owned = *bun_core::heap::take(key.lookup);
let mut head = owned.head;
DNSLookup::process_get_addr_info_native(&raw mut head, err, ptr::null_mut());
let Some(mut array) =
super::options_jsc::result_any_to_js(result, global_object).transpose()
else {
// SAFETY: `key.lookup` is the heap-allocated request stored in the
// pending-cache slot; consumed via `heap::take` below.
unsafe {
let mut pending = (*key.lookup).head.next;
// Consume the request and move `head` out by value;
// `ptr::read` + `heap::take` would double-Drop `DNSLookup`.
Comment on lines +4324 to +4325

@github-actions github-actions Bot Aug 6, 2026

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

let owned = *bun_core::heap::take(key.lookup);
let mut head = owned.head;
DNSLookup::process_get_addr_info_native(&raw mut head, err, ptr::null_mut());

while let Some(value) = pending {
pending = (*value.as_ptr()).next;
DNSLookup::process_get_addr_info_native(
value.as_ptr(),
err,
ptr::null_mut(),
);
}
while let Some(value) = pending {
pending = (*value.as_ptr()).next;
DNSLookup::process_get_addr_info_native(value.as_ptr(), err, ptr::null_mut());
}
return;
}
return;
};
// SAFETY: `key.lookup` is the heap-allocated request stored in the
// pending-cache slot; consumed via `heap::take` below.
Expand All @@ -4326,25 +4341,27 @@ impl Resolver {
let mut prev_global = (*key.lookup).head.global_this();

{
array.ensure_still_alive();
ensure_result_still_alive(array);
DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));
array.ensure_still_alive();
ensure_result_still_alive(array);
}

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
pending = (*value.as_ptr()).next;
if !core::ptr::eq(prev_global, new_global) {
// The settle consumed an Err's pending exception; never reuse it.
if array.is_err() || !core::ptr::eq(prev_global, new_global) {
// The head conversion proved `result` is non-null.
array = super::options_jsc::result_any_to_js(result, new_global)
.unwrap_or(None)
.unwrap(); // TODO: properly propagate exception upwards
.transpose()
.unwrap();
prev_global = new_global;
}

array.ensure_still_alive();
ensure_result_still_alive(array);
DNSLookup::on_complete_with_array(value.as_ptr(), array);
array.ensure_still_alive();
ensure_result_still_alive(array);
}
}
}
Expand Down Expand Up @@ -4390,26 +4407,25 @@ impl Resolver {
// The callback need not and should not attempt to free the memory
// pointed to by hostent; the ares library will free it when the
// callback returns.
let mut array = super::cares_jsc::hostent_to_js_response(&mut *addr, prev_global, b"")
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
array.ensure_still_alive();
let mut array = super::cares_jsc::hostent_to_js_response(&mut *addr, prev_global, b"");
ensure_result_still_alive(array);
CAresReverse::on_complete(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));

array.ensure_still_alive();
ensure_result_still_alive(array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = super::cares_jsc::hostent_to_js_response(&mut *addr, new_global, b"")
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
// The settle consumed an Err's pending exception; never reuse it.
if array.is_err() || !core::ptr::eq(prev_global, new_global) {
array = super::cares_jsc::hostent_to_js_response(&mut *addr, new_global, b"");
prev_global = new_global;
}
pending = (*value.as_ptr()).next;

array.ensure_still_alive();
ensure_result_still_alive(array);
CAresReverse::on_complete(value.as_ptr(), array);
array.ensure_still_alive();
ensure_result_still_alive(array);
}
}
}
Expand Down Expand Up @@ -4453,26 +4469,25 @@ impl Resolver {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();

let mut array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, prev_global)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
array.ensure_still_alive();
let mut array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, prev_global);
ensure_result_still_alive(array);
CAresNameInfo::on_complete(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));

array.ensure_still_alive();
ensure_result_still_alive(array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, new_global)
.unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards
// The settle consumed an Err's pending exception; never reuse it.
if array.is_err() || !core::ptr::eq(prev_global, new_global) {
array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, new_global);
prev_global = new_global;
}
pending = (*value.as_ptr()).next;

array.ensure_still_alive();
ensure_result_still_alive(array);
CAresNameInfo::on_complete(value.as_ptr(), array);
array.ensure_still_alive();
ensure_result_still_alive(array);
}
}
}
Expand Down
Loading
Loading