Respect preconfigured OkHttp dispatcher #5212
Conversation
WalkthroughRemoved the explicit dispatcher assignment from OkHttp client construction so the builder uses its default dispatcher; added a test that verifies a preconfigured OkHttpClient retains its original Dispatcher instance via identity check. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt (1)
12-28: White‑box test correctly covers the regression; only minor optional nitsThe test nicely exercises the
preconfiguredpath and verifies dispatcher identity via the cache, which directly guards against regressions increateOkHttpClient.If you want to tighten things a bit, one optional tweak would be to give
clientCachea more precise type when casting (e.g.,MutableMap<HttpTimeoutConfig?, OkHttpClient>with an explicit@Suppress("UNCHECKED_CAST")), so future readers immediately see what key/value types are expected rather than working from the engine source. Otherwise this looks good to me.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt(1 hunks)ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt
🔇 Additional comments (1)
ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt (1)
144-154: Respecting preconfigured dispatcher looks correct; please double‑check dispatcher lifecycle expectationsConditionally skipping
builder.dispatcher(Dispatcher())whenconfig.preconfigured != nullcorrectly preserves the caller’s dispatcher configuration while keeping the previous behavior for the non‑preconfigured path.One thing to keep in mind: cloned clients built from a preconfigured
OkHttpClientwill now share that sameDispatcherinstance, and the engine’s close logic still callsclient.dispatcher.executorService.shutdown()for all cached clients. That means closing the engine will also shut down the executor backing the caller’s dispatcher if it is shared elsewhere. Please confirm this lifecycle coupling is acceptable for the intended use cases ofpreconfigured, or document it if that’s the contract you want.
|
|
||
| builder.dispatcher(Dispatcher()) | ||
| if (config.preconfigured == null) { | ||
| builder.dispatcher(Dispatcher()) |
There was a problem hiding this comment.
Probably we should just drop this line as OkHttp already creates Dispatcher by default:
internal var dispatcher: Dispatcher = Dispatcher()
There was a problem hiding this comment.
Probably we should just drop this line as
OkHttpalready createsDispatcherby default:internal var dispatcher: Dispatcher = Dispatcher()
Yes, because even when copying okHttpClientPrototype, Dispatcher should be shared, as stated in the docs:
OkHttp performs best when you create a single OkHttpClient instance and reuse it for all of your HTTP calls. This is because each client holds its own connection pool and thread pools. Reusing connections and threads reduces latency and saves memory. Conversely, creating a client for each request wastes resources on idle pools.
The above should apply to prototype client also, new Dispatcher should NEVER be set.
|
It's important to understand that "newBuilder()" method creates new OkHttpClient instance, but reuses original Dispatcher and ConnectionPool. (which is expected and recommended in the docs). That is why it's important to remove setting new Dispatcher completely, even in the "prototype" instance case, to achieve that all Ktor OkHttpEngine instances reuse the same Dispatcher and ConnectionPool. EDIT: If somebody doesn't want to share Dispatcher and ConnectionPool instances across OkHttpEngine instances, that can be achieved by explicitly setting DIFFERENT PRECONFIGURED INSTANCES when creating Engine instances, but that should not be the default behaviour. TLDR: You should just make a "sane default", but leave all the options open to the developer, not force any of your decisions on us. |
osipxd
force-pushed
the
codex/fix-issue-4908-in-ktor
branch
from
e08f89f to
966c5de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt (2)
14-21: Consider adding resource cleanup.The test creates an
OkHttpEnginebut doesn't close it. IfOkHttpEngineimplementsCloseable(which engines typically do), consider wrapping the engine usage in auseblock or adding explicit cleanup to prevent resource leaks in the test.🔎 View suggested fix
@Test fun usesPreconfiguredDispatcher() { val dispatcher = Dispatcher() val preconfiguredClient = OkHttpClient.Builder() .dispatcher(dispatcher) .build() - val engine = OkHttpEngine(OkHttpConfig().apply { preconfigured = preconfiguredClient }) - val cacheField = engine.javaClass.getDeclaredField("clientCache").apply { isAccessible = true } - val clientCache = cacheField.get(engine) as Map<*, *> + OkHttpEngine(OkHttpConfig().apply { preconfigured = preconfiguredClient }).use { engine -> + val cacheField = engine.javaClass.getDeclaredField("clientCache").apply { isAccessible = true } + val clientCache = cacheField.get(engine) as Map<*, *> - val client = clientCache[null] as OkHttpClient + val client = clientCache[null] as OkHttpClient - assertSame(dispatcher, client.dispatcher) + assertSame(dispatcher, client.dispatcher) + } }
22-28: Test correctly verifies dispatcher preservation.The reflection-based approach is appropriate for verifying internal behavior when no public API exists. The use of
assertSamecorrectly validates instance identity rather than equality, which is exactly what's needed to confirm the preconfigured dispatcher is reused rather than replaced.The test directly addresses the PR objective and validates the fix for issue #4908.
Optional: Consider adding documentation.
While the reflection approach is justified, a brief comment explaining why reflection is necessary (no public API to verify dispatcher preservation) and what the
nullkey represents in the clientCache could help future maintainers understand the test's assumptions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt(0 hunks)ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports forio.ktor.*packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with@InternalAPIannotation
Run./gradlew lintKotlinand fix all linting issues before giving control back to the user
Use./gradlew formatKotlinto automatically fix formatting issues
Run./gradlew updateLegacyAbiafter making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the/api/directories
Validate ABI with./gradlew checkLegacyAbiand update with./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions
Files:
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt
**/test/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
Follow test naming pattern:
DescribeWhatIsBeingTested
Files:
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt
🧠 Learnings (2)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt
🔇 Additional comments (2)
ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt (2)
1-3: LGTM!Standard Apache 2.0 license header is correctly formatted.
5-10: LGTM!Package declaration and imports are appropriate for the test.
Closes #4908
Summary
Testing
Codex Task