Skip to content

dns: give each waiter of a shared query its own result #44649

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

Open
robobun wants to merge 2 commits into
base: main
Choose a base branch
from
Open

dns: give each waiter of a shared query its own result #44649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d4697d
dns: give each waiter of a shared query its own result
robobun Oct 6, 2026
707f54a
test(dns): bound the gate wait in the termination test
robobun Oct 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
203 changes: 84 additions & 119 deletions src/runtime/dns_jsc/dns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,8 +1711,7 @@ impl DNSLookup {
/// construction from a live `&JSGlobalObject` (never null) and is a
/// JSC_BORROW backref — the global outlives every `DNSLookup` it spawns.
/// The pointee is the JSC heap global, not memory owned by `self`, so the
/// returned `&` remains valid even after `self` is dropped (drain loops
/// rely on this when caching the ref across `heap::take`).
/// returned `&` remains valid even after `self` is dropped.
#[inline]
fn global_this(&self) -> &JSGlobalObject {
self.global_this.get()
Expand Down Expand Up @@ -1856,12 +1855,7 @@ impl DNSLookup {
}
}

/// The converted answer for one global, shared by every waiter of a
/// pending-cache entry on that global. A conversion that threw is turned into
/// its exception value *once* (the first `reject(Err(Thrown))` would take it
/// off the VM and leave nothing for the next waiter); a termination settles
/// nobody.
#[derive(Clone, Copy)]
/// Not `Copy`: each waiter of a shared query settles with its own conversion of the reply.
pub(crate) enum Outcome {
Value(JSValue),
Error(JSValue),
Expand All @@ -1886,12 +1880,18 @@ impl Outcome {
}
}

/// Each waiter's completion may allocate; keep the shared value alive across them.
#[inline]
fn keep_alive(&self) {
if let Outcome::Value(v) | Outcome::Error(v) = self {
v.ensure_still_alive();
/// After a `Stopped` conversion the rest of the chain is `Stopped` too and converts nothing.
fn for_waiter(
chain_stopped: &mut bool,
global: &JSGlobalObject,
convert: impl FnOnce() -> JsResult<JSValue>,
) -> Outcome {
if *chain_stopped {
return Outcome::Stopped;
}
let outcome = Outcome::of(global, convert());
*chain_stopped = matches!(outcome, Outcome::Stopped);
outcome
}

/// The resolver backends' completion callbacks (c-ares poll, libinfo,
Expand All @@ -1915,11 +1915,6 @@ impl Outcome {
}
}

#[inline]
fn keep_alive(outcome: &Outcome) {
outcome.keep_alive();
}

impl Drop for DNSLookup {
fn drop(&mut self) {
bun_output::scoped_log!(DNSLookup, "deinit");
Expand Down Expand Up @@ -4330,26 +4325,25 @@ impl Resolver {
// pending-cache slot; consumed via `heap::take` below.
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();
let mut array =
Outcome::of(prev_global, addr.to_js_response(prev_global, T::TYPE_NAME));
keep_alive(&array);
CAresLookup::<T>::on_complete(ptr::addr_of_mut!((*key.lookup).head), array);
let mut stopped = false;
let global = (*key.lookup).head.global_this();
CAresLookup::<T>::on_complete(
ptr::addr_of_mut!((*key.lookup).head),
Outcome::for_waiter(&mut stopped, global, || {
addr.to_js_response(global, T::TYPE_NAME)
}),
);
drop(bun_core::heap::take(key.lookup));

keep_alive(&array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = Outcome::of(new_global, addr.to_js_response(new_global, T::TYPE_NAME));
prev_global = new_global;
}
let global = (*value.as_ptr()).global_this();
pending = (*value.as_ptr()).next;

keep_alive(&array);
CAresLookup::<T>::on_complete(value.as_ptr(), array);
keep_alive(&array);
CAresLookup::<T>::on_complete(
value.as_ptr(),
Outcome::for_waiter(&mut stopped, global, || {
addr.to_js_response(global, T::TYPE_NAME)
}),
Comment on lines 4338 to +4345

@claude claude Bot Oct 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.

🟡 (optional) Any process that opens many connections to one host at once now stalls its event loop for the whole burst, where the base settled the burst in one conversion. Every chained waiter runs addr.to_js_response again inside the single c-ares callback at dns.rs:4343-4345 (same shape at dns.rs:4405, 4465, 4529, 4580), and PendingCacheKey::append puts no cap on the chain. Fix: bound the per-callback work for a shared query, e.g. convert once per global and give each later waiter a structural copy (new array and record objects, shared strings), or cap the chain and start a new wire query past it, so a 10,000-waiter burst stays near the base's 4 ms rather than 157 ms.

Why this was flagged

The trigger is N overlapping identical lookups on one resolver: node:net connect, node:http without keep-alive, a pool warm-up or a Bun.ModuleGraph fan-out all route through dns.lookup and chain on one pending-cache slot. On the base the drain converted once per global (old prev_global branch at dns.rs:4331-4342 of the base) and settled all N waiters with that value, so the callback cost was O(records). After the change each waiter's for_waiter closure at dns.rs:4343 and siblings rebuilds the full answer (per PR: 7,566 instructions, 9 mallocs and 5 JS cells per extra 2-address waiter; 157 ms vs 4 ms for 10,000 waiters on 50 records), all inside one c-ares poll callback with no yield. Every other timer, socket and promise on that loop waits for it. The dismissal accepted the author's cost table as a trade-off; the author's own description asks whether to accept it or add a structural copy, which is the remedy.

Verification: Triggering condition: N overlapping identical lookups on one resolver that chain onto one pending-cache slot before the reply arrives. Outcome::for_waiter (src/runtime/dns_jsc/dns.rs:1884-1895) runs convert() for every waiter; the base converted once per distinct global. PendingCacheKey::append (dns.rs:1022-1030) only links the node onto the tail; there is no cap on chain length.

@robobun robobun Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the trade-off that the Downsides section puts to the reviewer. The numbers for it are in the Notes. Two points on the framing:

  • A burst of net.connect() to one hostname does chain on one lookup (lookupAndConnect in src/js/node/net.ts calls dns.lookup), and each connection now gets its own conversion. That cost is small beside the connects. 500 connections to localhost for each batch, release builds, 6 alternating runs: 61.9 ms on main (59.3 to 64.1) and 63.3 ms here (59.4 to 68.7). The 157 ms case is 10,000 waiters on one 50-record answer.
  • A cap on the chain sends more wire queries for one name. This PR keeps the existing behavior that identical in-flight lookups share one query.

The structural copy stays the follow-up if a maintainer wants the lower cost for each waiter: convert once into a value that no caller receives, then give each waiter a new array and new records with the strings shared. Outcome stays move-only in that shape too.

);
}
}
}
Expand Down Expand Up @@ -4390,34 +4384,28 @@ impl Resolver {
// slot; `addr` is the c-ares-allocated AddrInfo freed by `_free_addr` below.
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();
let mut array = Outcome::of(
prev_global,
super::cares_jsc::addr_info_to_js_array(&mut *addr, prev_global),
);
let mut stopped = false;
let global = (*key.lookup).head.global_this();
// 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));
keep_alive(&array);
DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), array);
DNSLookup::on_complete_with_array(
ptr::addr_of_mut!((*key.lookup).head),
Outcome::for_waiter(&mut stopped, global, || {
super::cares_jsc::addr_info_to_js_array(&mut *addr, global)
}),
);
drop(bun_core::heap::take(key.lookup));

keep_alive(&array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = Outcome::of(
new_global,
super::cares_jsc::addr_info_to_js_array(&mut *addr, new_global),
);
prev_global = new_global;
}
let global = (*value.as_ptr()).global_this();
pending = (*value.as_ptr()).next;

keep_alive(&array);
DNSLookup::on_complete_with_array(value.as_ptr(), array);
keep_alive(&array);
DNSLookup::on_complete_with_array(
value.as_ptr(),
Outcome::for_waiter(&mut stopped, global, || {
super::cares_jsc::addr_info_to_js_array(&mut *addr, global)
}),
);
}
}
}
Expand All @@ -4434,7 +4422,7 @@ impl Resolver {

let _guard = self.ref_guard();

let mut array: Outcome = match super::options_jsc::result_any_to_js(result, global_object)
let head: Outcome = match super::options_jsc::result_any_to_js(result, global_object)
.transpose()
{
Some(a) => Outcome::of(global_object, a),
Expand Down Expand Up @@ -4465,31 +4453,21 @@ impl Resolver {
// pending-cache slot; consumed via `heap::take` below.
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();

{
keep_alive(&array);
DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));
keep_alive(&array);
}
let mut stopped = matches!(head, Outcome::Stopped);
DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), head);
drop(bun_core::heap::take(key.lookup));

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
let global = (*value.as_ptr()).global_this();
pending = (*value.as_ptr()).next;
if !core::ptr::eq(prev_global, new_global) {
// Non-null addrinfo (checked above): never `None`.
array = Outcome::of(
new_global,
super::options_jsc::result_any_to_js(result, new_global)
.map(|a| a.expect("addrinfo present")),
);
prev_global = new_global;
}

keep_alive(&array);
DNSLookup::on_complete_with_array(value.as_ptr(), array);
keep_alive(&array);
DNSLookup::on_complete_with_array(
value.as_ptr(),
Outcome::for_waiter(&mut stopped, global, || {
// Non-null addrinfo (checked above): never `None`.
super::options_jsc::result_any_to_js(result, global)
.map(|a| a.expect("addrinfo present"))
}),
);
}
}
}
Expand Down Expand Up @@ -4530,34 +4508,28 @@ impl Resolver {
// slot; `addr` is the c-ares-owned hostent (freed by c-ares after the callback).
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();
let mut stopped = false;
let global = (*key.lookup).head.global_this();
// 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 = Outcome::of(
prev_global,
super::cares_jsc::hostent_to_js_response(&mut *addr, prev_global, b""),
CAresReverse::on_complete(
ptr::addr_of_mut!((*key.lookup).head),
Outcome::for_waiter(&mut stopped, global, || {
super::cares_jsc::hostent_to_js_response(&mut *addr, global, b"")
}),
);
keep_alive(&array);
CAresReverse::on_complete(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));

keep_alive(&array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = Outcome::of(
new_global,
super::cares_jsc::hostent_to_js_response(&mut *addr, new_global, b""),
);
prev_global = new_global;
}
let global = (*value.as_ptr()).global_this();
pending = (*value.as_ptr()).next;

keep_alive(&array);
CAresReverse::on_complete(value.as_ptr(), array);
keep_alive(&array);
CAresReverse::on_complete(
value.as_ptr(),
Outcome::for_waiter(&mut stopped, global, || {
super::cares_jsc::hostent_to_js_response(&mut *addr, global, b"")
}),
);
}
}
}
Expand Down Expand Up @@ -4598,32 +4570,25 @@ impl Resolver {
// pending-cache slot; consumed via `heap::take` below.
unsafe {
let mut pending = (*key.lookup).head.next;
let mut prev_global = (*key.lookup).head.global_this();

let mut array = Outcome::of(
prev_global,
super::cares_jsc::nameinfo_to_js_response(&mut name_info, prev_global),
let mut stopped = false;
let global = (*key.lookup).head.global_this();
CAresNameInfo::on_complete(
ptr::addr_of_mut!((*key.lookup).head),
Outcome::for_waiter(&mut stopped, global, || {
super::cares_jsc::nameinfo_to_js_response(&mut name_info, global)
}),
);
keep_alive(&array);
CAresNameInfo::on_complete(ptr::addr_of_mut!((*key.lookup).head), array);
drop(bun_core::heap::take(key.lookup));

keep_alive(&array);

while let Some(value) = pending {
let new_global = (*value.as_ptr()).global_this();
if !core::ptr::eq(prev_global, new_global) {
array = Outcome::of(
new_global,
super::cares_jsc::nameinfo_to_js_response(&mut name_info, new_global),
);
prev_global = new_global;
}
let global = (*value.as_ptr()).global_this();
pending = (*value.as_ptr()).next;

keep_alive(&array);
CAresNameInfo::on_complete(value.as_ptr(), array);
keep_alive(&array);
CAresNameInfo::on_complete(
value.as_ptr(),
Outcome::for_waiter(&mut stopped, global, || {
super::cares_jsc::nameinfo_to_js_response(&mut name_info, global)
}),
);
}
}
}
Expand Down
Loading
Loading