Respect preconfigured OkHttp dispatcher

[e5l]

Closes #4908

Summary

• avoid overriding the dispatcher when using a preconfigured OkHttpClient
• add a JVM test to ensure the engine keeps the preconfigured dispatcher

Testing

• ./gradlew :ktor-client-okhttp:jvmTest --tests io.ktor.client.engine.okhttp.OkHttpEngineTest

---

Codex Task

[coderabbitai]

Walkthrough

The change modifies OkHttpEngine's client initialization to conditionally assign a dispatcher. When a preconfigured OkHttpClient is provided via config, the default dispatcher setup is skipped. A new test verifies that preconfigured clients retain their original dispatcher instance.

Changes

Cohort / File(s)	Summary
OkHttpEngine initialization refactor ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpEngine.kt	Modified createOkHttpClient to conditionally set dispatcher only when config.preconfigured is null; preconfigured clients now bypass dispatcher assignment.
OkHttpEngine dispatcher behavior test ktor-client/ktor-client-okhttp/jvm/test/io/ktor/client/engine/okhttp/OkHttpEngineTest.kt	Added new test class OkHttpEngineTest with usesPreconfiguredDispatcher() test method that verifies preconfigured OkHttpClient instances preserve their original dispatcher through reflection-based identity checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Dispatcher initialization logic: Verify that conditional dispatcher assignment correctly handles both preconfigured and default client paths, and confirm no unintended side effects on timeout, proxy, or config application.
• Reflection-based testing: Review the private field access and identity checks to ensure test reliability and compatibility with future refactoring.
• Initialization path behavior: Confirm that skipping dispatcher setup for preconfigured clients is the intended behavior and does not break existing callers.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: avoiding dispatcher override when using a preconfigured OkHttpClient, matching the core modification in the codebase.
Description check	✅ Passed	The PR description addresses a specific issue (#4908), explains the solution clearly, and provides testing instructions, but lacks explicit Subsystem and Motivation sections from the template.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/fix-issue-4908-in-ktor

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 nits

The test nicely exercises the preconfigured path and verifies dispatcher identity via the cache, which directly guards against regressions in createOkHttpClient.

If you want to tighten things a bit, one optional tweak would be to give clientCache a 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

📥 Commits

Reviewing files that changed from the base of the PR and between f5bd49d and 86bf0e4.

📒 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 expectations

Conditionally skipping builder.dispatcher(Dispatcher()) when config.preconfigured != null correctly 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 OkHttpClient will now share that same Dispatcher instance, and the engine’s close logic still calls client.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 of preconfigured, or document it if that’s the contract you want.

[osipxd]

Probably we should just drop this line as OkHttp already creates Dispatcher by default:

internal var dispatcher: Dispatcher = Dispatcher()

[gajicm93]

Probably we should just drop this line as OkHttp already creates Dispatcher by 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.

[gajicm93]

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.