Preserve caller context at async request dispatch - #990
Preserve caller context at async request dispatch#990sylvesterkaczmarek wants to merge 1 commit into
Conversation
sylvesterkaczmarek
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a0e4f8b80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val requestOptions = requestOptions.applyDefaults(RequestOptions.from(clientOptions)) | ||
| return request | ||
| .thenComposeAsync { clientOptions.httpClient.executeAsync(it, requestOptions) } | ||
| .thenCompose { clientOptions.httpClient.executeAsync(it, requestOptions) } |
There was a problem hiding this comment.
With more than 64 concurrent async calls, OkHttp's default Dispatcher.maxRequests keeps later calls in its internal ready queue and does not submit them to the configured executor until an earlier call finishes. That delayed submission occurs on the earlier call's completion thread, so a context-capturing dispatcherExecutorService captures the wrong request's context. Consequently, switching these sites to thenCompose preserves caller context only for calls immediately promoted by OkHttp; saturated clients can still lose or cross-contaminate request context, and the two-request regression test does not exercise this boundary.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #952.
Remove the unconditional common-pool hop between request preparation and asynchronous transport entry at all 295 dispatch sites. The configured transport can now capture the caller's context before scheduling HTTP work. Network I/O and stream callbacks retain their existing executors; no new executor, option, or dependency is introduced.
Regression coverage exercises typed and raw responses, the synchronous client's async view, configured and explicit stream callback executors, pending response futures, and synchronous/asynchronous transport failures.
Execution boundary
Synchronous setup inside
HttpClient.executeAsyncnow runs on the request-preparation completion thread. Transports remain responsible for offloading blocking I/O. This does not add general context propagation across transport queues, retries, asynchronous credentials, or application future chains.Validation
46bf1c1and pass with this change.SKIP_MOCK_TESTS=true ./gradlew :openai-java-core:test :openai-java-client-okhttp:test: 6,050 passed, 598 skipped.git diff --checkpass.The generator source is not present here; the same substitution is applied consistently to the checked-in service implementations.