Add **Shared ScheduledExecutorService** for timeouts#36
Conversation
…xecutorService`** for timeouts ## CopilotSession.java - Added `ScheduledExecutorService` import. - New field `timeoutScheduler`: shared single-thread scheduler, daemon thread named `sendAndWait-timeout`. - Initialized in 3-arg constructor. - `sendAndWait()`: replaced per-call `Executors.newSingleThreadScheduledExecutor()` with `timeoutScheduler.schedule()`. Cleanup calls `timeoutTask.cancel(false)` instead of `scheduler.shutdown()`. - `close()`: added `timeoutScheduler.shutdownNow()` before the blocking `session.destroy` RPC call so stale timeouts cannot fire after close. ## TimeoutEdgeCaseTest.java (new) - `testTimeoutDoesNotFireAfterSessionClose`: proves close() cancels pending timeouts (future not completed by stale TimeoutException). - `testSendAndWaitReusesTimeoutThread`: proves two sendAndWait calls share one scheduler thread instead of spawning two. - Uses reflection to construct a hanging `JsonRpcClient` (blocking InputStream, sink OutputStream). Signed-off-by: Ed Burns <edburns@microsoft.com>
|
01-mock-llm-to-show-blocking.md Prompts that produced this PR. |
…xecutorService`** for timeouts ## CopilotSession.java - Added `ScheduledExecutorService` import. - New field `timeoutScheduler`: shared single-thread scheduler, daemon thread named `sendAndWait-timeout`. - Initialized in 3-arg constructor. - `sendAndWait()`: replaced per-call `Executors.newSingleThreadScheduledExecutor()` with `timeoutScheduler.schedule()`. Cleanup calls `timeoutTask.cancel(false)` instead of `scheduler.shutdown()`. - `close()`: added `timeoutScheduler.shutdownNow()` before the blocking `session.destroy` RPC call so stale timeouts cannot fire after close. ## TimeoutEdgeCaseTest.java (new) - `testTimeoutDoesNotFireAfterSessionClose`: proves close() cancels pending timeouts (future not completed by stale TimeoutException). - `testSendAndWaitReusesTimeoutThread`: proves two sendAndWait calls share one scheduler thread instead of spawning two. - Uses reflection to construct a hanging `JsonRpcClient` (blocking InputStream, sink OutputStream). Signed-off-by: Ed Burns <edburns@microsoft.com>
There was a problem hiding this comment.
Pull request overview
This PR updates CopilotSession#sendAndWait to use a session-scoped ScheduledExecutorService instead of creating a new scheduler per call, and adds regression tests covering timeout behavior during close() and scheduler thread reuse.
Changes:
- Add a session-level
timeoutSchedulerand use it forsendAndWait()timeouts. - Update
close()to shut down the timeout scheduler before invoking the blockingsession.destroyRPC. - Add
TimeoutEdgeCaseTestto verify timeouts don’t fire afterclose()and that timeout scheduling reuses a single thread.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main/java/com/github/copilot/sdk/CopilotSession.java | Introduces a shared per-session scheduler for sendAndWait timeouts and shuts it down during close(). |
| src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java | Adds tests for timeout-after-close and for scheduler thread reuse across multiple sendAndWait calls. |
Comments suppressed due to low confidence (2)
src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java:125
- Second
Thread.sleep(100)has the same flakiness risk as the first. Consider using a bounded wait/polling approach instead of fixed sleeps to make the test deterministic.
Thread.sleep(100);
long afterSecond = countTimeoutThreads();
src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java:114
- If an assertion fails before reaching the explicit
session.close()at the end of the test, the session/scheduler thread can leak. Consider try-with-resources forCopilotSessionor afinallythat always closes it.
CopilotSession session = new CopilotSession("test-thread-count-id", rpc);
long baselineCount = countTimeoutThreads();
CompletableFuture<AssistantMessageEvent> result1 = session
.sendAndWait(new MessageOptions().setPrompt("hello1"), 30000);
Replace Executors.newSingleThreadScheduledExecutor with an explicit ScheduledThreadPoolExecutor so we can enable removeOnCancelPolicy(true). Without this, each call to sendAndWait() that completes normally cancels its timeout task, but the cancelled task remains in the scheduler's work queue, leaking memory over the lifetime of the session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ed Burns <edburns@microsoft.com>
pom.xml Add mockito-core 5.17.0 as a test dependency. src/main/java/com/github/copilot/sdk/CopilotSession.java Replace Executors.newSingleThreadScheduledExecutor with explicit ScheduledThreadPoolExecutor and enable removeOnCancelPolicy(true) so cancelled timeout tasks are purged from the work queue immediately. Wrap timeoutScheduler.schedule() in a try-catch for RejectedExecutionException. On rejection (close() race), the event subscription is cleaned up and the returned future completes exceptionally instead of throwing uncaught. src/test/java/com/github/copilot/sdk/SchedulerShutdownRaceTest.java (new) TDD test that reproduces the scheduler shutdown race. Uses Mockito to stub JsonRpcClient.invoke(), then shuts down the scheduler without setting isTerminated, and asserts sendAndWait() returns a failed future rather than throwing RejectedExecutionException. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ed Burns <edburns@microsoft.com>
|
@edburns let me know when you finish clearing the feedback from Copilot on this PR, then flag this PR as Ready for Review and I'll take a look! |
…Wait src/main/java/com/github/copilot/sdk/CopilotSession.java Skip scheduling the timeout task when timeoutMs <= 0, matching the Javadoc contract that 0 or negative means "no timeout". Previously, timeoutMs=0 would schedule an immediate timeout, contradicting the docs. The timeout cancel in the whenComplete cleanup is now guarded for the null case. src/test/java/com/github/copilot/sdk/ZeroTimeoutContractTest.java (new) TDD test that asserts the documented contract: calling sendAndWait with timeoutMs=0 should not cause the future to complete with a TimeoutException. Uses Mockito to stub JsonRpcClient.invoke(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java Wrap CopilotSession in try-with-resources in both tests so the session and its scheduler thread are always cleaned up, even if an assertion fails before the explicit close() call. In test 1, the explicit session.close() is kept because it is the action under test; the try-with-resources provides a safety net via idempotent double-close. In test 2, the explicit session.close() is removed since it was purely cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ScheduledExecutorService** for timeoutsScheduledExecutorService** for timeouts
|
@brunoborges , this is now ready for your review. I'd appreciate your input. This is preparatory work for allowing the API to accept an |
|
Marking this as Draft in preparation for introducing the same PR but from a topic branch on this repository, rather than from a topic branch on a fork of this repository. |
|
Is superseded by #38 |
562527f
CopilotSession.java
ScheduledExecutorServiceimport.timeoutScheduler: shared single-thread scheduler, daemon thread namedsendAndWait-timeout.sendAndWait(): replaced per-callExecutors.newSingleThreadScheduledExecutor()withtimeoutScheduler.schedule(). Cleanup callstimeoutTask.cancel(false)instead ofscheduler.shutdown().close(): addedtimeoutScheduler.shutdownNow()before the blockingsession.destroyRPC call so stale timeouts cannot fire after close.TimeoutEdgeCaseTest.java (new)
testTimeoutDoesNotFireAfterSessionClose: proves close() cancels pending timeouts (future not completed by stale TimeoutException).testSendAndWaitReusesTimeoutThread: proves two sendAndWait calls share one scheduler thread instead of spawning two.JsonRpcClient(blocking InputStream, sink OutputStream).Resolves #ISSUE_NUMBER