Accept optional Executor on CopilotClientOptions to mitigate pool starvation#40
Accept optional Executor on CopilotClientOptions to mitigate pool starvation#40
Conversation
…nc calls through it; shared timeout scheduler. src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java - Added `Executor` field, `getExecutor()`, fluent `setExecutor(Executor)` with pending-null guard, and clone support. src/main/java/com/github/copilot/sdk/CopilotClient.java - Extracted `startCoreBody()` from `startCore()` lambda; `supplyAsync` uses provided executor when non-null. - `stop()` routes session-close `runAsync` through provided executor when non-null. - Passes executor to `RpcHandlerDispatcher` constructor. - Sets executor on new sessions via `session.setExecutor()` in `createSession` and `resumeSession`. src/main/java/com/github/copilot/sdk/RpcHandlerDispatcher.java - Added `Executor` field and 3-arg constructor. - All 5 `CompletableFuture.runAsync()` calls now go through private `runAsync(Runnable)` helper that uses executor when non-null. src/main/java/com/github/copilot/sdk/CopilotSession.java - Added `Executor` field and package-private `setExecutor()`. - Replaced per-call `ScheduledExecutorService` with shared `timeoutScheduler` (daemon thread, shut down in `close()`). - `executeToolAndRespondAsync` and `executePermissionAndRespondAsync` use executor when non-null. src/test/java/com/github/copilot/sdk/ExecutorWiringTest.java (new) - 6 E2E tests using `TrackingExecutor` decorator to verify all `*Async` paths route through the provided executor: client start, tool call, permission, user input, hooks, and client stop. src/test/java/com/github/copilot/sdk/RpcHandlerDispatcherTest.java - Updated constructor call to pass `null` for new executor parameter.
…8695-virtual-threads-accept-executor # Conflicts: # src/main/java/com/github/copilot/sdk/CopilotSession.java
modified: src/main/java/com/github/copilot/sdk/CopilotClient.java - Spotless. modified: src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java - Remove stub from TDD red phase. modified: src/site/markdown/cookbook/multiple-sessions.md - Document new feature. modified: src/test/java/com/github/copilot/sdk/ExecutorWiringTest.java - Update test documentation. Signed-off-by: Ed Burns <edburns@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Adds support for supplying a caller-provided Executor to route the SDK’s internal CompletableFuture.*Async work off ForkJoinPool.commonPool(), reducing starvation risk when running multiple blocking sessions concurrently.
Changes:
- Introduces
CopilotClientOptions.getExecutor()/setExecutor(Executor)(nullable) and includes it inclone(). - Wires the executor through
CopilotClientstart/stop paths,RpcHandlerDispatcher, andCopilotSessiontool/permission response execution. - Adds E2E coverage (
ExecutorWiringTest) and documents the new option in the multiple-sessions cookbook.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java | Adds nullable Executor option and clones it. |
| src/main/java/com/github/copilot/sdk/CopilotClient.java | Uses provided executor for startCore() and stop() async work; passes executor into dispatcher and sessions. |
| src/main/java/com/github/copilot/sdk/RpcHandlerDispatcher.java | Adds executor-aware async dispatch helper for server-to-client handler execution. |
| src/main/java/com/github/copilot/sdk/CopilotSession.java | Routes broadcast tool/permission response tasks through provided executor when present. |
| src/test/java/com/github/copilot/sdk/ExecutorWiringTest.java | New E2E tests asserting executor usage across key async dispatch paths. |
| src/test/java/com/github/copilot/sdk/RpcHandlerDispatcherTest.java | Updates dispatcher construction for new constructor signature. |
| src/site/markdown/cookbook/multiple-sessions.md | Documents pool starvation and how to provide a custom executor. |
Comments suppressed due to low confidence (1)
src/main/java/com/github/copilot/sdk/CopilotSession.java:789
- Same as tool execution: submitting this permission-response task via CompletableFuture.runAsync(task, executor) can throw RejectedExecutionException synchronously if the provided executor rejects tasks. Consider handling submission failure to ensure the pending permission request is either answered (e.g., denied) or otherwise completed deterministically.
if (executor != null) {
CompletableFuture.runAsync(task, executor);
} else {
CompletableFuture.runAsync(task);
}
Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/93199b25-7c90-4c45-9540-527396b8990c Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/63b9b09f-f1f4-44d3-8e34-ad01e355cc6a Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
…sion Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/63b9b09f-f1f4-44d3-8e34-ad01e355cc6a Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
|
@edburns should we throw an IAE if the user passes null? |
|
@brunoborges wrote:
We don't do that for other setters in |
modified: README.md - Use the "uncomment these three lines to get Virtual Threads" approach modified: src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java - Cleanup. Sorting. Signed-off-by: Ed Burns <edburns@microsoft.com>
There was no deep thinking before. Making null-safe now leads us to a more stable and strict API, and towards GA. What I mean is that this is perhaps now the moment to make the API safer. |
|
@brunoborges wrote:
Of course. I'll take this as consent to add the null checks throughout, at least in the I'll take the remainder of the null checks as a separate issue. |
Before the change?
CompletableFuture.*Asynccalls usedForkJoinPool.commonPool(), which could become starved when multiple sessions blocked concurrently.CopilotClientOptionshad no way for callers to supply a custom thread pool.RejectedExecutionExceptionthrown synchronously by any async submission site would propagate to callers unexpectedly, potentially leaving JSON-RPC requests unanswered or skipping connection cleanup.After the change?
CopilotClientOptionshas a newExecutorfield with fluentsetExecutor(Executor)/getExecutor()accessors. Passingnullpreserves the originalForkJoinPool.commonPool()behaviour.CopilotClientstart/stop,RpcHandlerDispatcher(all 5runAsynccall sites via a private helper), andCopilotSession(executeToolAndRespondAsync/executePermissionAndRespondAsync).RejectedExecutionExceptionand handle it context-sensitively:CopilotClient.startCore()— returnsCompletableFuture.failedFuture(e)so rejection surfaces through the returned future instead of throwing synchronously.CopilotClient.stop()— falls back to closing each session inline so cleanup always completes even if the executor has been shut down.RpcHandlerDispatcher.runAsync()— falls back to running the handler inline on the notification thread so the JSON-RPC request is never left unanswered.CopilotSession.executeToolAndRespondAsync()andexecutePermissionAndRespondAsync()— fall back to running the task inline so pending tool/permission calls always receive a reply.ExecutorWiringTest(6 tests) verifies the executor is used for client start, tool-call dispatch, permission dispatch, user-input dispatch, hooks dispatch, and client stop.RpcHandlerDispatcherTestupdated for the new 3-arg constructor (passesnull).Pull request checklist
mvn spotless:applyhas been run to format the codemvn clean verifypasses locallyDoes this introduce a breaking change?