refactor: migrate hickory-client to hickory-net#1396
Merged
Conversation
The hickory-client crate has been subsumed into hickory-net per the hickory-dns release notes. No future releases of hickory-client are expected. - Replace hickory-client 0.25 dep in clash-dns with hickory-net 0.26 - Update feature flags (https-aws-lc-rs, h3-aws-lc-rs, etc.) to point to hickory-net instead of hickory-client - Add missing DnsUdpSocket and AsyncIoTokioAsStd imports in runtime.rs - Update dns_client.rs to use Client<DnsRuntimeProvider> (now generic) - Remove proto 0.25/0.26 encode/decode roundtrip workaround - Rewrite dns_stream_builder for new sync API: TcpClientStream::new and tls_client_connect now return (Future, BufDnsStreamHandle); await the future before passing to Client::new/with_timeout - tls_client_connect now takes ServerName<'static> instead of String - HttpsClientStream and H3ClientStream use from_sender pattern - Update handler.rs tests to use hickory_net/hickory_proto imports and new 0.26 connection API Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThis PR upgrades clash-rs from hickory-client 0.25 to hickory-net 0.26. Cargo manifests are updated to wire feature flags to the new crate; DNS client types are parameterized with ChangesHickory DNS Library Upgrade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Switch dhcproto from crates.io 0.14 to latest git ref (eece41c) which has been updated to use hickory-proto 0.26.1, eliminating the last duplicate hickory-proto 0.25.x in the dependency tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📊 Proxy Throughput ResultsShadowsocks
Trojan
VMess
VLESS
SOCKS5
AnyTLS
Hysteria2
TUIC
ShadowQUIC
SSH
Netem Tests (50 ms delay, 1% packet loss)Shadowsocks
Trojan
Hysteria2
TUIC
ShadowQUIC
Ran 34 variant(s) in parallel; each direction transfers the full payload. 🖥️ Test Environment
📎 View full workflow run and download artifacts Full test logDownload the |
…_default()
Instead of repeating #[cfg(feature = "aws-lc-rs")] / #[cfg(feature = "ring")]
at every call site, use the runtime-installed default provider:
- clash-dns/src/handler.rs: remove the two cfg-gated any_supported_type
imports; use CryptoProvider::get_default().key_provider.load_private_key()
- clash-lib/src/proxy/anytls/inbound/handler.rs: replace feature-gated
supported_verify_schemes() body with CryptoProvider::get_default()
- anytls/inbound/{handler,tls}.rs tests: replace local install_crypto_provider()
helpers that duplicated the feature-gated logic with a call to
crate::setup_default_crypto_provider()
The single source of truth for feature selection remains the centralized
setup_default_crypto_provider() functions in clash-lib/src/lib.rs and
clash-dns/src/lib.rs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clash-dns/src/handler.rs`:
- Around line 557-559: The current use of inspect_err with assert!(false, ...)
around stream_future.await is unidiomatic and masks proper error propagation;
remove the inspect_err call and simply await the future with the trailing ? so
the error propagates (i.e., assign stream = stream_future.await?), or if you
want a custom panic use .expect(...) on the awaited result instead; update the
expression that binds stream (where stream_future.await is used and inspect_err
is called) accordingly.
In `@clash-lib/src/app/dns/resolver/enhanced.rs`:
- Around line 858-868: Remove the redundant inner import inside the
test_bad_labels_with_custom_resolver test: delete the line "use
hickory_net::proto::{op, rr};" so the function uses the module-level
hickory_proto::{op, rr} imports instead, avoiding shadowing and redundancy while
leaving the rest of the test (name, op::Message::query(), etc.) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 454b05d1-ba27-4727-85b3-9b483aa2c46e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
clash-dns/Cargo.tomlclash-dns/src/handler.rsclash-lib/Cargo.tomlclash-lib/src/app/dns/dns_client.rsclash-lib/src/app/dns/resolver/enhanced.rsclash-lib/src/app/dns/runtime.rs
Comment on lines
+557
to
+559
| let stream = stream_future.await.inspect_err(|e| { | ||
| assert!(false, "Failed to connect to DoT server: {}", e); | ||
| })?; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Avoid assert!(false, …) inside inspect_err; let ? propagate the error.
Panicking inside inspect_err is unidiomatic — the trailing ? already fails the test with the error, so the assert is dead in the success path and just masks the propagation in the failure path. Drop the inspect_err (or replace the whole expression with .expect(...) if you want a tailored panic message).
♻️ Proposed simplification
- let stream = stream_future.await.inspect_err(|e| {
- assert!(false, "Failed to connect to DoT server: {}", e);
- })?;
+ let stream = stream_future
+ .await
+ .map_err(|e| anyhow::anyhow!("Failed to connect to DoT server: {e}"))?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clash-dns/src/handler.rs` around lines 557 - 559, The current use of
inspect_err with assert!(false, ...) around stream_future.await is unidiomatic
and masks proper error propagation; remove the inspect_err call and simply await
the future with the trailing ? so the error propagates (i.e., assign stream =
stream_future.await?), or if you want a custom panic use .expect(...) on the
awaited result instead; update the expression that binds stream (where
stream_future.await is used and inspect_err is called) accordingly.
Comment on lines
858
to
+868
| #[tokio::test] | ||
| async fn test_bad_labels_with_custom_resolver() { | ||
| // Use hickory_client::proto (0.25) types throughout so DnsRequest::new | ||
| // gets a compatible Message. TODO: remove shadowing once hickory-client | ||
| // 0.26.0 stable ships and aligns with hickory-proto 0.26. | ||
| use hickory_client::proto::{op, rr}; | ||
| use hickory_net::proto::{op, rr}; | ||
|
|
||
| let name = rr::Name::from_str_relaxed("some_domain.understore") | ||
| .unwrap() | ||
| .append_domain(&rr::Name::root()) | ||
| .unwrap(); | ||
| assert_eq!(name.to_string(), "some_domain.understore."); | ||
|
|
||
| let mut m = op::Message::new(); | ||
| let mut m = op::Message::query(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for a `proto` re-export inside hickory-net 0.26 by checking the published crate's lib.rs.
pip install --quiet requests >/dev/null 2>&1 || true
python - <<'PY'
import urllib.request, io, tarfile, sys
url = "https://crates.io/api/v1/crates/hickory-net/0.26.0/download"
data = urllib.request.urlopen(url).read()
with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as tf:
for m in tf.getmembers():
if m.name.endswith("src/lib.rs"):
f = tf.extractfile(m)
src = f.read().decode("utf-8", errors="replace")
print(m.name)
for i, line in enumerate(src.splitlines(), 1):
if "proto" in line and ("pub use" in line or "pub mod" in line or "extern crate" in line):
print(f"{i}: {line}")
break
PYRepository: Watfaq/clash-rs
Length of output: 2641
🏁 Script executed:
# Check Cargo.toml for hickory dependency versions
cat Cargo.toml | grep -A 10 "hickory"Repository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Read the actual import section and test function context from the file
sed -n '735,875p' clash-lib/src/app/dns/resolver/enhanced.rsRepository: Watfaq/clash-rs
Length of output: 4929
🌐 Web query:
hickory-net 0.26 proto module re-export
💡 Result:
In hickory-net (including the 0.26 line), the crate re-exports the hickory-proto crate as proto. Evidence: the hickory-net crate docs show under “Re-exports” the item hickory_proto pub use as proto; [1].
Citations:
Remove the redundant use hickory_net::proto::{op, rr}; import inside the test function.
The module-level imports (lines 740-743) already bring hickory_proto::{op, rr} into scope. The inner import on line 860 redundantly re-imports the same items via hickory_net's re-export of hickory_proto as proto. While the code compiles successfully, the inner import should be removed to avoid unnecessary shadowing and improve clarity.
♻️ Proposed cleanup
#[tokio::test]
async fn test_bad_labels_with_custom_resolver() {
- use hickory_net::proto::{op, rr};
-
let name = rr::Name::from_str_relaxed("some_domain.understore")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[tokio::test] | |
| async fn test_bad_labels_with_custom_resolver() { | |
| // Use hickory_client::proto (0.25) types throughout so DnsRequest::new | |
| // gets a compatible Message. TODO: remove shadowing once hickory-client | |
| // 0.26.0 stable ships and aligns with hickory-proto 0.26. | |
| use hickory_client::proto::{op, rr}; | |
| use hickory_net::proto::{op, rr}; | |
| let name = rr::Name::from_str_relaxed("some_domain.understore") | |
| .unwrap() | |
| .append_domain(&rr::Name::root()) | |
| .unwrap(); | |
| assert_eq!(name.to_string(), "some_domain.understore."); | |
| let mut m = op::Message::new(); | |
| let mut m = op::Message::query(); | |
| #[tokio::test] | |
| async fn test_bad_labels_with_custom_resolver() { | |
| let name = rr::Name::from_str_relaxed("some_domain.understore") | |
| .unwrap() | |
| .append_domain(&rr::Name::root()) | |
| .unwrap(); | |
| assert_eq!(name.to_string(), "some_domain.understore."); | |
| let mut m = op::Message::query(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clash-lib/src/app/dns/resolver/enhanced.rs` around lines 858 - 868, Remove
the redundant inner import inside the test_bad_labels_with_custom_resolver test:
delete the line "use hickory_net::proto::{op, rr};" so the function uses the
module-level hickory_proto::{op, rr} imports instead, avoiding shadowing and
redundancy while leaving the rest of the test (name, op::Message::query(), etc.)
unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clash-dns/src/handler.rs`:
- Around line 35-41: The TLS CertifiedKey creation currently panics on missing
crypto provider or unsupported key; make this fallible by replacing the
expect(...) calls in the CertifiedKey::new argument construction (the calls to
rustls::crypto::CryptoProvider::get_default() and key_provider.load_private_key)
with checked logic that returns a propagated error (io::Error or DNSError) from
get_dns_listener instead of aborting; update the call site that constructs the
CertifiedKey to return Err(...) on failure and change the three listener
registration sites that currently use .into() to use .try_into()? so failures
bubble up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e91da45-9134-405f-bc25-25d64954badf
📒 Files selected for processing (3)
clash-dns/src/handler.rsclash-lib/src/proxy/anytls/inbound/handler.rsclash-lib/src/proxy/anytls/inbound/tls.rs
Comment on lines
+35
to
+41
| CertifiedKey::new( | ||
| pair.certs, | ||
| rustls::crypto::CryptoProvider::get_default() | ||
| .expect("no default crypto provider installed") | ||
| .key_provider | ||
| .load_private_key(pair.key) | ||
| .expect("unsupported private key type"), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "handler.rs" -path "*/clash-dns/*"Repository: Watfaq/clash-rs
Length of output: 86
🏁 Script executed:
head -100 clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 3708
🏁 Script executed:
sed -n '30,50p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 837
🏁 Script executed:
wc -l clash-dns/src/handler.rsRepository: Watfaq/clash-rs
Length of output: 88
🏁 Script executed:
sed -n '1,200p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 7781
🏁 Script executed:
grep -n "get_dns_listener" clash-dns/src/handler.rsRepository: Watfaq/clash-rs
Length of output: 185
🏁 Script executed:
grep -n "\.into()" clash-dns/src/handler.rs | head -20Repository: Watfaq/clash-rs
Length of output: 521
🏁 Script executed:
grep -n "CertificateKeyPair" clash-dns/src/handler.rsRepository: Watfaq/clash-rs
Length of output: 359
🏁 Script executed:
sed -n '203,230p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 1109
🏁 Script executed:
sed -n '260,280p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 932
🏁 Script executed:
sed -n '300,320p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 921
🏁 Script executed:
sed -n '340,360p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 953
🏁 Script executed:
sed -n '203,230p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 1109
🏁 Script executed:
sed -n '265,275p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 511
🏁 Script executed:
grep -A 3 "register_https_listener\|register_tls_listener\|register_h3_listener" clash-dns/src/handler.rs | head -30Repository: Watfaq/clash-rs
Length of output: 525
🏁 Script executed:
sed -n '203,225p' clash-dns/src/handler.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 944
Avoid panicking on TLS provider/key setup in the listener path.
This runs in the production DoH/DoT/DoH3 startup path, so a missing default CryptoProvider or an unsupported key now aborts the process instead of returning a config error through get_dns_listener. Make this conversion fallible and propagate an io::Error/DNSError back to the caller.
♻️ Suggested direction
-impl From<CertificateKeyPair> for Arc<dyn rustls::server::ResolvesServerCert> {
- fn from(pair: CertificateKeyPair) -> Self {
- Arc::new(AlwaysResolvesServerRawPublicKeys::new(Arc::new(
- CertifiedKey::new(
- pair.certs,
- rustls::crypto::CryptoProvider::get_default()
- .expect("no default crypto provider installed")
- .key_provider
- .load_private_key(pair.key)
- .expect("unsupported private key type"),
- ),
- )))
- }
-}
+impl TryFrom<CertificateKeyPair> for Arc<dyn rustls::server::ResolvesServerCert> {
+ type Error = std::io::Error;
+
+ fn try_from(pair: CertificateKeyPair) -> Result<Self, Self::Error> {
+ let provider = rustls::crypto::CryptoProvider::get_default()
+ .ok_or_else(|| std::io::Error::other("no default crypto provider installed"))?;
+ let key = provider
+ .key_provider
+ .load_private_key(pair.key)
+ .map_err(|e| {
+ std::io::Error::new(
+ std::io::ErrorKind::InvalidInput,
+ format!("unsupported private key type: {e}"),
+ )
+ })?;
+
+ Ok(Arc::new(AlwaysResolvesServerRawPublicKeys::new(Arc::new(
+ CertifiedKey::new(pair.certs, key),
+ ))))
+ }
+}Then switch the three registration sites from .into() to .try_into()?.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clash-dns/src/handler.rs` around lines 35 - 41, The TLS CertifiedKey creation
currently panics on missing crypto provider or unsupported key; make this
fallible by replacing the expect(...) calls in the CertifiedKey::new argument
construction (the calls to rustls::crypto::CryptoProvider::get_default() and
key_provider.load_private_key) with checked logic that returns a propagated
error (io::Error or DNSError) from get_dns_listener instead of aborting; update
the call site that constructs the CertifiedKey to return Err(...) on failure and
change the three listener registration sites that currently use .into() to use
.try_into()? so failures bubble up.
- clash-dns/src/handler.rs: move TokioRuntimeProvider import from hickory_proto::runtime to hickory_net::runtime (it was never in proto); fix A record comparison: A has no ::new() in 0.26, use ip.0 == Ipv4Addr - clash-lib/src/app/dns/resolver/enhanced.rs: replace removed Message::set_recursion_desired() with direct field m.metadata.recursion_desired; replace Client::connect().await (removed) with Client::from_sender() (sync); replace DnsRequest::set_id() (removed) with req.metadata.id = ... Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- handler.rs: H3ClientStreamBuilder::builder() → H3ClientStream::builder() (builder() is a static method on H3ClientStream, not H3ClientStreamBuilder); import H3ClientStream alongside H3ClientStreamBuilder - handler.rs: response.answers() → &response.answers (now a pub field in 0.26.1) - handler.rs: answers[0].data() → &answers[0].data (pub field; method requires RecordData trait in scope, field access is simpler) - enhanced.rs: bring DnsHandle and FirstAnswer traits into test scope so client.send() and .first_answer() resolve correctly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clash-lib/src/app/dns/resolver/enhanced.rs`:
- Around line 858-887: The test test_bad_labels_with_custom_resolver currently
depends on the public resolver at 1.1.1.1:53 which makes it flaky in CI; either
run it against a local DNS test server or exclude it from default runs—update
the test declaration (fn test_bad_labels_with_custom_resolver) to use a local
mock resolver (replace UdpClientStream/DnsRuntimeProvider setup with a local
test server equivalent) or simply add the #[ignore] attribute so CI won't run it
by default; ensure any spawned background client (client::Client::from_sender /
tokio::spawn(bg)) still compiles with the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 66b041d4-5440-4163-98ee-ecd58829eb91
📒 Files selected for processing (2)
clash-dns/src/handler.rsclash-lib/src/app/dns/resolver/enhanced.rs
Comment on lines
858
to
887
| #[tokio::test] | ||
| async fn test_bad_labels_with_custom_resolver() { | ||
| // Use hickory_client::proto (0.25) types throughout so DnsRequest::new | ||
| // gets a compatible Message. TODO: remove shadowing once hickory-client | ||
| // 0.26.0 stable ships and aligns with hickory-proto 0.26. | ||
| use hickory_client::proto::{op, rr}; | ||
| use hickory_net::proto::{op, rr}; | ||
|
|
||
| let name = rr::Name::from_str_relaxed("some_domain.understore") | ||
| .unwrap() | ||
| .append_domain(&rr::Name::root()) | ||
| .unwrap(); | ||
| assert_eq!(name.to_string(), "some_domain.understore."); | ||
|
|
||
| let mut m = op::Message::new(); | ||
| let mut m = op::Message::query(); | ||
| let mut q = op::Query::new(); | ||
|
|
||
| q.set_name(name); | ||
| q.set_query_type(rr::RecordType::A); | ||
| m.add_query(q); | ||
| m.set_recursion_desired(true); | ||
| m.metadata.recursion_desired = true; | ||
|
|
||
| let stream = UdpClientStream::builder( | ||
| "1.1.1.1:53".parse().unwrap(), | ||
| DnsRuntimeProvider::new_direct(None, None), | ||
| ) | ||
| .build(); | ||
| let (client, bg) = client::Client::connect(stream).await.unwrap(); | ||
|
|
||
| let (client, bg) = client::Client::<DnsRuntimeProvider>::from_sender(stream); | ||
| tokio::spawn(bg); | ||
|
|
||
| let mut req = DnsRequest::new(m, DnsRequestOptions::default()); | ||
| req.set_id(rand::random::<u16>()); | ||
| req.metadata.id = rand::random::<u16>(); | ||
| let res = client.send(req).first_answer().await; | ||
| assert!(res.is_ok()); |
Contributor
There was a problem hiding this comment.
Keep this live-network test out of the default suite.
This still depends on the public resolver at 1.1.1.1:53, so it can fail in offline or rate-limited CI while the other network-dependent tests in this module are already ignored. Either back it with a local test server or mark it ignored as well.
♻️ Minimal containment
#[tokio::test]
+ #[ignore = "uses public DNS and is flaky in CI"]
async fn test_bad_labels_with_custom_resolver() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[tokio::test] | |
| async fn test_bad_labels_with_custom_resolver() { | |
| // Use hickory_client::proto (0.25) types throughout so DnsRequest::new | |
| // gets a compatible Message. TODO: remove shadowing once hickory-client | |
| // 0.26.0 stable ships and aligns with hickory-proto 0.26. | |
| use hickory_client::proto::{op, rr}; | |
| use hickory_net::proto::{op, rr}; | |
| let name = rr::Name::from_str_relaxed("some_domain.understore") | |
| .unwrap() | |
| .append_domain(&rr::Name::root()) | |
| .unwrap(); | |
| assert_eq!(name.to_string(), "some_domain.understore."); | |
| let mut m = op::Message::new(); | |
| let mut m = op::Message::query(); | |
| let mut q = op::Query::new(); | |
| q.set_name(name); | |
| q.set_query_type(rr::RecordType::A); | |
| m.add_query(q); | |
| m.set_recursion_desired(true); | |
| m.metadata.recursion_desired = true; | |
| let stream = UdpClientStream::builder( | |
| "1.1.1.1:53".parse().unwrap(), | |
| DnsRuntimeProvider::new_direct(None, None), | |
| ) | |
| .build(); | |
| let (client, bg) = client::Client::connect(stream).await.unwrap(); | |
| let (client, bg) = client::Client::<DnsRuntimeProvider>::from_sender(stream); | |
| tokio::spawn(bg); | |
| let mut req = DnsRequest::new(m, DnsRequestOptions::default()); | |
| req.set_id(rand::random::<u16>()); | |
| req.metadata.id = rand::random::<u16>(); | |
| let res = client.send(req).first_answer().await; | |
| assert!(res.is_ok()); | |
| #[tokio::test] | |
| #[ignore = "uses public DNS and is flaky in CI"] | |
| async fn test_bad_labels_with_custom_resolver() { | |
| use hickory_net::proto::{op, rr}; | |
| let name = rr::Name::from_str_relaxed("some_domain.understore") | |
| .unwrap() | |
| .append_domain(&rr::Name::root()) | |
| .unwrap(); | |
| assert_eq!(name.to_string(), "some_domain.understore."); | |
| let mut m = op::Message::query(); | |
| let mut q = op::Query::new(); | |
| q.set_name(name); | |
| q.set_query_type(rr::RecordType::A); | |
| m.add_query(q); | |
| m.metadata.recursion_desired = true; | |
| let stream = UdpClientStream::builder( | |
| "1.1.1.1:53".parse().unwrap(), | |
| DnsRuntimeProvider::new_direct(None, None), | |
| ) | |
| .build(); | |
| let (client, bg) = client::Client::<DnsRuntimeProvider>::from_sender(stream); | |
| tokio::spawn(bg); | |
| let mut req = DnsRequest::new(m, DnsRequestOptions::default()); | |
| req.metadata.id = rand::random::<u16>(); | |
| let res = client.send(req).first_answer().await; | |
| assert!(res.is_ok()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clash-lib/src/app/dns/resolver/enhanced.rs` around lines 858 - 887, The test
test_bad_labels_with_custom_resolver currently depends on the public resolver at
1.1.1.1:53 which makes it flaky in CI; either run it against a local DNS test
server or exclude it from default runs—update the test declaration (fn
test_bad_labels_with_custom_resolver) to use a local mock resolver (replace
UdpClientStream/DnsRuntimeProvider setup with a local test server equivalent) or
simply add the #[ignore] attribute so CI won't run it by default; ensure any
spawned background client (client::Client::from_sender / tokio::spawn(bg)) still
compiles with the chosen approach.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
clash-dns/src/handler.rs (1)
32-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPanicking
Fromimpl is still on the listener startup path.This conversion is invoked by
register_https_listener,register_tls_listener, andregister_h3_listener(lines 269, 310, 350) via.into(), so a missing defaultCryptoProvideror an unsupported key now aborts the process at server start rather than surfacing as aDNSErrorfromget_dns_listener. Recommend converting this toTryFromand switching the three call sites to.try_into()?so failures propagate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clash-dns/src/handler.rs` around lines 32 - 45, Replace the infallible From<CertificateKeyPair> implementation with a TryFrom<CertificateKeyPair> that returns Result<Arc<dyn rustls::server::ResolvesServerCert>, DNSError> and propagate failures from CryptoProvider::get_default() and key_provider.load_private_key(...) instead of calling expect; construct and return the Arc via AlwaysResolvesServerRawPublicKeys::new(CertifiedKey::new(...)) on success. Then update the three call sites register_https_listener, register_tls_listener, and register_h3_listener to use .try_into()? (or handle the Result and map to DNSError) so missing crypto provider or unsupported key types surface as DNSError from get_dns_listener rather than panicking at startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@clash-dns/src/handler.rs`:
- Around line 32-45: Replace the infallible From<CertificateKeyPair>
implementation with a TryFrom<CertificateKeyPair> that returns Result<Arc<dyn
rustls::server::ResolvesServerCert>, DNSError> and propagate failures from
CryptoProvider::get_default() and key_provider.load_private_key(...) instead of
calling expect; construct and return the Arc via
AlwaysResolvesServerRawPublicKeys::new(CertifiedKey::new(...)) on success. Then
update the three call sites register_https_listener, register_tls_listener, and
register_h3_listener to use .try_into()? (or handle the Result and map to
DNSError) so missing crypto provider or unsupported key types surface as
DNSError from get_dns_listener rather than panicking at startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 130ff7c6-5fa6-4552-b788-f685c7eb02ce
📒 Files selected for processing (2)
clash-dns/src/handler.rsclash-lib/src/app/dns/resolver/enhanced.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
hickory-clientcrate has been subsumed intohickory-netper the hickory-dns release notes. No future releases ofhickory-clientare expected.Changes
hickory-client 0.25dep inclash-dnswithhickory-net 0.26https-aws-lc-rs,h3-aws-lc-rs, etc.) to point tohickory-netDnsUdpSocketandAsyncIoTokioAsStdimports inruntime.rsdns_client.rsto useClient<DnsRuntimeProvider>(now generic in 0.26)hickory-clientpinned to proto 0.25 while the rest used 0.26)dns_stream_builderfor new sync API:TcpClientStream::newandtls_client_connectnow return(Future, BufDnsStreamHandle); await the future before passing stream toClient::new/with_timeouttls_client_connectnow takesServerName<'static>instead ofStringClient::from_senderpatternhandler.rstests to usehickory_net/hickory_protoimports and new 0.26 connection APIResult
hickory-clientis fully removed from the dependency tree. The remaininghickory-proto 0.25.2is pulled bydhcproto(pre-existing, unrelated).Summary by CodeRabbit
Refactor
Tests
Chores