KTOR-9554 Add dns config to OkHttp engine#5570
Conversation
📝 WalkthroughWalkthroughAdds an optional ChangesOkHttp DNS configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt`:
- Around line 36-53: Tests create OkHttpEngine instances
(OkHttpEngine(OkHttpConfig()...) in the two test methods but never close them,
leaking dispatcher/executor resources; update both tests (`default dns is
preserved when dns config is not set` and the earlier test using customDns) to
ensure the engine is closed when the test finishes — either wrap engine creation
in a try/finally and call engine.close() in finally or use a resource-safe
construct so that the OkHttpEngine instance is closed after extracting
clientCache and performing assertions; reference the OkHttpEngine constructor,
OkHttpConfig usage and the clientCache reflection access when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 725984e3-f7fe-47d8-8a1a-b0f5488b7c79
📒 Files selected for processing (4)
ktor-client/ktor-client-okhttp/api/ktor-client-okhttp.apiktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpConfig.ktktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.ktktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt
bjhham
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I wouldn't say it covers the ticket, however. The ticket refers to the address resolution in ktor-network which is used in the CIO engine. I'll create a new ticket with reference to supporting DNS configuration in OkHttp that better reflects the work done here.
bjhham
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I wouldn't say it covers the ticket, however. The ticket refers to the address resolution in ktor-network which is used in the CIO engine. I'll create a new ticket with reference to supporting DNS configuration in OkHttp that better reflects the work done here.
|
Here's the new ticket to reflect the update to the OkHttp engine: KTOR-9554 DNS configuration for OkHttp client engine |
f6c88d9 to
12bca6c
Compare
dns config to OkHttp enginedns config to OkHttp engine
|
Thanks for the review! |
Subsystem
Client / OkHttp engine
Motivation
KTOR-462 / #1678 / #1924
OkHttp exposes a
Dnsinterface butOkHttpConfigdoes not surface it as a first-class option. Users currently have to drop down toengine { config { dns(...) } }to inject a custom resolver (DNS-over-HTTPS, host overrides for tests, etc.).Solution
Add
OkHttpConfig.dns: Dns?and apply it inOkHttpEngine.createOkHttpClientright afterproxy, mirroring the existing pattern. Defaults tonull→Dns.SYSTEMis used as before.Scoped to the OkHttp engine; the broader unified resolver abstraction (KTOR-462) is out of scope here. The same per-engine pattern can follow for Apache5 in a separate PR.