fix: remove ring from aws-lc-rs code path (~500KB-1MB binary savings)#1397
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughUpdated clash-lib Cargo feature forwarding and dependency options to align AWS‑LC and ring rustls variants across quinn, rcgen, and shadowquic; refined test imports in clash-dns; integration tests now use ClashInstance::start(...) instead of thread-based startup. ChangesTLS Backend Feature and Test Lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
When aws-lc-rs is the active crypto backend, ring was still being pulled in as a duplicate dependency by three paths: 1. quinn (v0.11) - used feature = "rustls" which aliases to "rustls-ring". Removed "rustls" from static features; now routes via aws-lc-rs feature (quinn/rustls-aws-lc-rs) or ring feature (quinn/rustls-ring). 2. rcgen - used default features which include ring backend. Switched to default-features = false; crypto backend now selected via aws-lc-rs/ring features (rcgen/aws_lc_rs or rcgen/ring). 3. shadowquic - hardcoded ring throughout. Switched to the fork ibigbug/shadowquic@a6f3fde which adds an aws-lc-rs feature flag (see spongebob888/shadowquic#134). Added default-features = false on the dep and explicit feature routing in aws-lc-rs/ring clash-lib features. Result: cargo tree -p clash-lib -F aws-lc-rs,shadowquic -i ring returns empty — ring is completely absent from the aws-lc-rs path. Estimated binary size reduction: ~500KB-1MB stripped release. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ibigbug
force-pushed
the
fix/remove-ring-from-aws-lc-rs
branch
from
9bd7323 to
55f6d79
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The anytls tests were using std::thread::spawn + start_clash without any cleanup mechanism. Threads kept running after each test completed, leaving ports 8902/9092 and 8998/9095 bound. The subsequent integration_test_anytls_udp test would then fail to bind port 8902 with 'Address already in use'. Replace both anytls tests with ClashInstance, which cancels the shutdown token on drop and waits for all ports to be released before returning — matching the pattern used throughout api_tests.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: 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/tests/integration_tests.rs`:
- Around line 138-158: Tests call ClashInstance::start which triggers TLS setup
and can race under aws-lc-rs; add a module-level initializer that calls
rustls::crypto::aws_lc_rs::default_provider().install_default() once (use
std::sync::Once) and invoke it at the start of affected tests (before calling
ClashInstance::start in integration_tests.rs) so the AWS-LC provider is
installed exactly once across tests.
🪄 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: 4ad77269-edf5-40b5-a45b-dadbdf80396f
📒 Files selected for processing (1)
clash-lib/tests/integration_tests.rs
Comment on lines
+138
to
+158
| let _server = ClashInstance::start( | ||
| Options { | ||
| config: Config::File(server_config.to_string_lossy().to_string()), | ||
| cwd: Some(wd_server.to_string_lossy().to_string()), | ||
| rt: None, | ||
| log_file: None, | ||
| }) | ||
| .expect("Failed to start AnyTLS server"); | ||
| }); | ||
| }, | ||
| vec![8902, 9092], | ||
| ) | ||
| .expect("Failed to start AnyTLS server"); | ||
|
|
||
| std::thread::spawn(move || { | ||
| start_clash(Options { | ||
| let _client = ClashInstance::start( | ||
| Options { | ||
| config: Config::File(client_config.to_string_lossy().to_string()), | ||
| cwd: Some(wd_client.to_string_lossy().to_string()), | ||
| rt: None, | ||
| log_file: None, | ||
| }) | ||
| .expect("Failed to start AnyTLS client"); | ||
| }); | ||
| }, | ||
| vec![8998, 9095], | ||
| ) | ||
| .expect("Failed to start AnyTLS client"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether aws-lc provider install is present before AnyTLS starts in this test file.
rg -n --type rust -C2 'install_default|integration_test_anytls|integration_test_anytls_udp|ClashInstance::start' clash-lib/tests/integration_tests.rsRepository: Watfaq/clash-rs
Length of output: 1635
🏁 Script executed:
# Check entire integration_tests.rs file for any TLS provider installations
rg -n "install_default|rustls::crypto" clash-lib/tests/integration_tests.rsRepository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Count total lines and show the overall structure of test functions in the file
wc -l clash-lib/tests/integration_tests.rs && echo "---" && rg -n "^async fn integration_test" clash-lib/tests/integration_tests.rsRepository: Watfaq/clash-rs
Length of output: 335
🏁 Script executed:
# Show lines around 117 (start of integration_test_anytls) through 200 to see the full first test
sed -n '117,200p' clash-lib/tests/integration_tests.rs | head -50Repository: Watfaq/clash-rs
Length of output: 1846
🏁 Script executed:
# Search for module-level setup functions, once patterns, or feature gates
rg -n "static|Once|#\[cfg" clash-lib/tests/integration_tests.rs | head -20Repository: Watfaq/clash-rs
Length of output: 94
🏁 Script executed:
# Get lines 1-50 to see file header, imports, and any setup
head -50 clash-lib/tests/integration_tests.rsRepository: Watfaq/clash-rs
Length of output: 1700
🏁 Script executed:
# Check if any other test functions also use ClashInstance with AnyTLS configs
rg -n "anytls|ClashInstance::start" clash-lib/tests/integration_tests.rsRepository: Watfaq/clash-rs
Length of output: 626
Install the AWS-LC rustls provider before AnyTLS startup in these tests.
ClashInstance::start(...) on line 138 and line 223 enter TLS code paths; without installing the aws-lc provider first, these tests can fail/flap under aws-lc-rs feature runs.
Per coding guidelines (**/*test*.rs), rustls::crypto::aws_lc_rs::default_provider().install_default() must be called before constructing TLS configs in tests. Add a module-level helper function with std::sync::Once to ensure single initialization across both affected test functions:
Suggested patch
+fn install_tls_provider_for_tests() {
+ #[cfg(feature = "aws-lc-rs")]
+ {
+ static INIT: std::sync::Once = std::sync::Once::new();
+ INIT.call_once(|| {
+ let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();
+ });
+ }
+}
+
#[tokio::test(flavor = "current_thread")]
#[serial_test::serial]
/// Test AnyTLS inbound and outbound functionality (ephemeral self-signed cert)
async fn integration_test_anytls() {
+ install_tls_provider_for_tests();
let wd_server =
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/data/config/server");
@@
#[tokio::test(flavor = "current_thread")]
#[serial_test::serial]
/// Test AnyTLS UDP-over-TCP v2: send a UDP datagram through SOCKS5 UDP
/// ASSOCIATE → AnyTLS inbound → echo server, verify round-trip payload.
async fn integration_test_anytls_udp() {
+ install_tls_provider_for_tests();
use std::net::SocketAddr;
use tokio::net::UdpSocket;🤖 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/tests/integration_tests.rs` around lines 138 - 158, Tests call
ClashInstance::start which triggers TLS setup and can race under aws-lc-rs; add
a module-level initializer that calls
rustls::crypto::aws_lc_rs::default_provider().install_default() once (use
std::sync::Once) and invoke it at the start of affected tests (before calling
ClashInstance::start in integration_tests.rs) so the AWS-LC provider is
installed exactly once across tests.
This was referenced May 13, 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.
Summary
When
aws-lc-rsis the active crypto backend,ringwas still being compiled in as a duplicate dependency via three paths. This PR eliminates all of them.Result:
cargo tree -p clash-lib -F aws-lc-rs,shadowquic -i ringreturns empty — ring is completely absent from theaws-lc-rscode path. Estimated binary size reduction: ~500KB–1MB stripped release.Root causes fixed
1.
quinn(v0.11) —rustlsfeature aliases torustls-ring2.
rcgen— default features includeringbackend3.
shadowquic— hardcodedringthroughoutShadowquic unconditionally required ring in all sub-crates (quinn-jls, iroh-quinn, rustls-jls, etc.) and in source code.
Switched to a fork that adds a proper
aws-lc-rsfeature flag: spongebob888/shadowquic#134Once spongebob888/shadowquic#134 merges, the dep can be pointed back to upstream.
Verification
Summary by CodeRabbit