Skip to content

fix(ext/node): implement dns Resolver cancel() and timeout support #33580

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

Merged
bartlomieju merged 9 commits into from
Apr 28, 2026
Merged

fix(ext/node): implement dns Resolver cancel() and timeout support #33580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae26eaf
fix(ext/node): implement dns Resolver cancel() and timeout support
bartlomieju Apr 27, 2026
4992b21
merge main
bartlomieju Apr 27, 2026
022cbab
fix: address review feedback from #33579
bartlomieju Apr 27, 2026
8ba1688
fix: replace em dash with ASCII hyphen in comment
bartlomieju Apr 27, 2026
906b839
fix: add pendingQueries tracking to getHostByAddr for cancel support
bartlomieju Apr 27, 2026
6a099a2
fix: disable dns cancel/timeout tests on Linux
bartlomieju Apr 27, 2026
e4a548f
fix: use cancel handles to properly abort DNS ops on timeout
bartlomieju Apr 27, 2026
6883e55
fix: cancel in-flight DNS ops and use short resolver timeout
bartlomieju Apr 28, 2026
669f583
chore: format
bartlomieju Apr 28, 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
10 changes: 9 additions & 1 deletion ext/net/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ pub async fn op_dns_resolve(
cancel_rid,
} = args;

let (config, opts) = if let Some(name_server) =
let (config, mut opts) = if let Some(name_server) =
options.as_ref().and_then(|o| o.name_server.as_ref())
{
let group = NameServerConfigGroup::from_ips_clear(
Expand All @@ -993,6 +993,14 @@ pub async fn op_dns_resolve(
system_conf::read_system_conf()?
};

// When a cancel handle is provided, use a short resolver timeout so
// that hickory's background connection tasks clean up quickly after
// cancellation (they are not aborted by the cancel handle itself).
if cancel_rid.is_some() {
opts.timeout = std::time::Duration::from_secs(1);
opts.attempts = 1;
}

{
let mut s = state.borrow_mut();
let perm = s.borrow_mut::<PermissionsContainer>();
Expand Down
4 changes: 1 addition & 3 deletions ext/node/ops/dns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,7 @@ fn assert_success_err(code: i32) -> DnsError {

use crate::ops::constant;

if code == 0 {
return DnsError::Io(std::io::Error::other("unexpected success code"));
}
debug_assert!(code != 0, "assert_success_err called with success code");

#[cfg(unix)]
let err = match code {
Expand Down
109 changes: 104 additions & 5 deletions ext/node/polyfills/internal_binding/cares_wrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
} from "ext:deno_node/internal_binding/async_wrap.ts";
import { ares_strerror } from "ext:deno_node/internal_binding/ares.ts";
import { notImplemented } from "ext:deno_node/_utils.ts";
import { core } from "ext:core/mod.js";
import {
op_dns_resolve,
op_net_get_ips_from_perm_token,
Expand Down Expand Up @@ -241,6 +242,8 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
#servers: [string, number][] | null = null;
#timeout: number;
#tries: number;
#pendingQueries: Set<QueryReqWrap> = new Set();
#cancelRids: Set<number> = new Set();

constructor(timeout: number, tries: number) {
super(providerType.DNSCHANNEL);
Expand Down Expand Up @@ -269,7 +272,10 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
ttl,
));

if (code === 0 || code === codeMap.get("EAI_NODATA")!) {
if (
code === 0 || code === codeMap.get("EAI_NODATA")! ||
code === "ETIMEOUT"
) {
break;
}
}
Expand All @@ -286,31 +292,52 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
resolveOptions?: Deno.ResolveDnsOptions,
ttl?: boolean,
): Promise<{
code: number;
// deno-lint-ignore no-explicit-any
code: any;
// deno-lint-ignore no-explicit-any
ret: any[];
}> {
let ret = [];
let code = 0;
// deno-lint-ignore no-explicit-any
let code: any = 0;

// Always create a cancel handle so cancel() can abort in-flight ops.
const cancelRid = core.createCancelHandle();
this.#cancelRids.add(cancelRid);
let timer: ReturnType<typeof setTimeout> | undefined;

try {
if (this.#timeout >= 0) {
timer = setTimeout(() => {
this.#cancelRids.delete(cancelRid);
core.tryClose(cancelRid);
}, this.#timeout);
}

const res = await op_dns_resolve({
query,
recordType,
options: resolveOptions,
cancelRid,
}, /* useEdns0 */ false);
if (ttl) {
ret = res;
} else {
ret = res.map((recordWithTtl) => recordWithTtl.data);
}
} catch (e) {
if (e instanceof Deno.errors.NotFound) {
if (e instanceof Deno.errors.Interrupted) {
code = "ETIMEOUT";
} else if (e instanceof Deno.errors.NotFound) {
code = codeMap.get("EAI_NODATA")!;
} else {
// TODO(cmorten): map errors to appropriate error codes.
code = codeMap.get("UNKNOWN")!;
}
} finally {
if (timer !== undefined) clearTimeout(timer);
this.#cancelRids.delete(cancelRid);
core.tryClose(cancelRid);
}

return { code, ret };
Expand All @@ -322,6 +349,7 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
//
// Ideally we move to using the "ANY" / "*" DNS query in future
// REF: https://github.com/denoland/deno/issues/14492
this.#pendingQueries.add(req);
(async () => {
const records: { type: Deno.RecordType; [key: string]: unknown }[] = [];

Expand Down Expand Up @@ -417,6 +445,9 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}),
]);

if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const err = records.length ? 0 : codeMap.get("EAI_NODATA")!;

req.oncomplete(err, records);
Expand All @@ -426,7 +457,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryA(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "A", req.ttl).then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

let recordsWithTtl;
if (req.ttl) {
recordsWithTtl = (ret as Deno.RecordWithTtl[]).map((val) => ({
Expand All @@ -442,7 +477,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryAaaa(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "AAAA", req.ttl).then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

let recordsWithTtl;
if (req.ttl) {
recordsWithTtl = (ret as Deno.RecordWithTtl[]).map((val) => ({
Expand All @@ -458,7 +497,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryCaa(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "CAA").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as Deno.CaaRecord[]).map(
({ critical, tag, value }) => ({
[tag]: value,
Expand All @@ -473,15 +516,23 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryCname(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "CNAME").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

req.oncomplete(code, ret);
});

return 0;
}

queryMx(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "MX").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as Deno.MxRecord[]).map(
({ preference, exchange }) => ({
priority: preference,
Expand All @@ -496,7 +547,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryNaptr(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "NAPTR").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as Deno.NaptrRecord[]).map(
({ order, preference, flags, services, regexp, replacement }) => ({
flags,
Expand All @@ -515,7 +570,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryNs(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "NS").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as string[]).map((record) => fqdnToHostname(record));

req.oncomplete(code, records);
Expand All @@ -525,7 +584,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryPtr(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "PTR").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as string[]).map((record) => fqdnToHostname(record));

req.oncomplete(code, records);
Expand All @@ -535,7 +598,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

querySoa(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "SOA").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

let record = {};

if (ret.length) {
Expand All @@ -560,7 +627,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

querySrv(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "SRV").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as Deno.SrvRecord[]).map(
({ priority, weight, port, target }) => ({
priority,
Expand All @@ -577,7 +648,11 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

queryTxt(req: QueryReqWrap, name: string): number {
this.#pendingQueries.add(req);
this.#query(name, "TXT").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

req.oncomplete(code, ret);
});

Expand All @@ -602,6 +677,16 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
for (let j = 0; j < missing; j++) {
expanded.push("0000");
}
} else if (part !== "" && part.includes(".")) {
// IPv4-mapped IPv6 (e.g. ::ffff:1.2.3.4) - convert dotted
// quad to two 16-bit hex groups
const octets = part.split(".").map(Number);
expanded.push(
((octets[0] << 8) | octets[1]).toString(16).padStart(4, "0"),
);
expanded.push(
((octets[2] << 8) | octets[3]).toString(16).padStart(4, "0"),
);
} else if (part !== "") {
expanded.push(part.padStart(4, "0"));
}
Expand All @@ -613,7 +698,12 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
return 0;
}

this.#pendingQueries.add(req);

this.#query(reverseName, "PTR").then(({ code, ret }) => {
if (!this.#pendingQueries.has(req)) return;
this.#pendingQueries.delete(req);

const records = (ret as string[]).map((record) => fqdnToHostname(record));
req.oncomplete(code, records);
});
Expand Down Expand Up @@ -649,7 +739,16 @@ export class ChannelWrap extends AsyncWrap implements ChannelWrapQuery {
}

cancel() {
notImplemented("cares.ChannelWrap.prototype.cancel");
for (const req of this.#pendingQueries) {
req.oncomplete("ECANCELLED", []);
Copy link
Copy Markdown
Contributor

@fibibot fibibot Apr 27, 2026

Choose a reason for hiding this comment

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

cancel() only walks #pendingQueries and fires req.oncomplete("ECANCELLED", []) synchronously — it doesn't actually abort the underlying op_dns_resolve. For the timeout path the cancel handle does abort the op, but here (no timeout set) the op stays alive until hickory's own timeout (~5s default × attempts) fires.

Failure pattern in CI on linux-aarch64 (https://github.com/denoland/deno/actions/runs/25011070179/job/73247390033):

[Warning] node_compat::parallel::test-dns-cancel-reverse-lookup.js was flaky on run 0
[Warning] ... was flaky on run 1
test test-dns-cancel-reverse-lookup.js has been running for more than 60 seconds
test test-dns-cancel-reverse-lookup.js ... fail (10002ms)

The user-facing callback fires ECANCELLED immediately, but the orphaned op_dns_resolve keeps the test's event loop alive until hickory times out. The 10002ms pretty clearly aligns with hickory's default 5s × 2 attempts.

Fix: extend the cancel-handle pattern to all queries, not just the timeout path. Roughly — give every #query call a cancelRid up front, store it on the req (or in a parallel map), and have cancel() core.tryClose each one in addition to (or instead of) firing oncomplete. Something like:

#cancelRids: Map<QueryReqWrap, number> = new Map();

// in #query, always create a cancelRid:
const cancelRid = core.createCancelHandle();
this.#cancelRids.set(req, cancelRid);  // populated by caller

// cancel():
for (const [req, rid] of this.#cancelRids) {
  core.tryClose(rid);
  req.oncomplete("ECANCELLED", []);
}
this.#cancelRids.clear();
this.#pendingQueries.clear();

The op should then return Deno.errors.Interrupted and the existing catch in #query will short-circuit, so the test's event loop can drain.

(There may be additional issues if req.oncomplete("ECANCELLED", []) doesn't get mapped through dnsException to set .code/.syscall/.hostname correctly in the test's assertion — worth verifying that string code passes through the existing dispatch, since dnsException is typically built around integer error codes.)

Copy link
Copy Markdown
Member Author

@bartlomieju bartlomieju Apr 28, 2026

Choose a reason for hiding this comment

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

This is already addressed in the current code -- cancel() now walks #cancelRids and calls core.tryClose(rid) on each one (lines 748-751), in addition to firing oncomplete("ECANCELLED", []) on pending queries. Every #query call creates a cancel handle via core.createCancelHandle() (line 305), stores it in #cancelRids (line 306), and cleans it up on completion (lines 312, 339). So orphaned op_dns_resolve calls are properly aborted and the event loop can drain.

}
this.#pendingQueries.clear();

// Abort in-flight DNS operations so the process can exit.
for (const rid of this.#cancelRids) {
core.tryClose(rid);
}
this.#cancelRids.clear();
}
}

Expand Down
4 changes: 4 additions & 0 deletions tests/node_compat/config.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,10 @@
// TODO(bartlomieju): this test was disabled during work on `node:inspector`, it never worked
// "parallel/test-disable-sigusr1.js": {},
"parallel/test-disable-sigusr1.js": {},
"parallel/test-dns-cancel-reverse-lookup.js": {},
"parallel/test-dns-channel-cancel.js": {},
"parallel/test-dns-channel-cancel-promise.js": {},
"parallel/test-dns-channel-timeout.js": {},
Copy link
Copy Markdown
Contributor

@fibibot fibibot Apr 27, 2026

Choose a reason for hiding this comment

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

Removing linux: false from these four entries is the right end-state, but at least test-dns-cancel-reverse-lookup.js is still failing on linux-aarch64 in this commit (10s hang → fail per the inline above). Either restore the platform skip for that one entry temporarily, or block this PR on the underlying fix. The other three (channel-cancel, channel-cancel-promise, channel-timeout) appear to pass — channel-timeout shows up green in the test log (test-dns-channel-timeout.js ... ok (141ms)).

"parallel/test-dns-default-order-ipv4.js": {},
"parallel/test-dns-default-order-ipv6.js": {},
"parallel/test-dns-default-order-verbatim.js": {},
Expand Down
2 changes: 1 addition & 1 deletion tools/lint_plugins/no_deno_api_in_polyfills.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const EXPECTED_VIOLATIONS: Record<string, number> = {
"ext/node/polyfills/_fs/_fs_lstat.ts": 4,
"ext/node/polyfills/testing.ts": 2,
"ext/node/polyfills/internal/tty.js": 2,
"ext/node/polyfills/internal_binding/cares_wrap.ts": 2,
"ext/node/polyfills/internal_binding/cares_wrap.ts": 3,
"ext/node/polyfills/_fs/_fs_dir.ts": 2,
"ext/node/polyfills/child_process.ts": 2,
"ext/node/polyfills/worker_threads.ts": 1,
Expand Down
Loading