Skip to content

Add **Shared ScheduledExecutorService** for timeouts #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
7 commits merged into from
Mar 30, 2026
Merged

Add **Shared ScheduledExecutorService** for timeouts #36

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c405b7
On branch edburns/dd-2758695-virtual-threads Add **Shared `ScheduledE…
edburns Mar 27, 2026
a36d145
On branch edburns/dd-2758695-virtual-threads Add **Shared `ScheduledE…
edburns Mar 27, 2026
9e102bc
Fix memory leak from cancelled timeout tasks in CopilotSession
edburns Mar 27, 2026
a1668c7
Fix scheduler memory leak and close() race condition in CopilotSession
edburns Mar 27, 2026
a01f0b5
Honor documented contract: timeoutMs <= 0 means no timeout in sendAnd…
edburns Mar 27, 2026
9dc5933
Prevent CopilotSession leak on assertion failure in TimeoutEdgeCaseTest
edburns Mar 27, 2026
075df31
spotless
edburns Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
On branch edburns/dd-2758695-virtual-threads Add **Shared `ScheduledE… 
…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>
  • Loading branch information
@edburns
edburns committed Mar 27, 2026
commit a36d145b777971a22e6cfb25ba605de177f6351d
31 changes: 14 additions & 17 deletions src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ public int read() throws IOException {
};
ByteArrayOutputStream sinkOutput = new ByteArrayOutputStream();

var ctor = JsonRpcClient.class.getDeclaredConstructor(
InputStream.class, java.io.OutputStream.class, Socket.class, Process.class);
var ctor = JsonRpcClient.class.getDeclaredConstructor(InputStream.class, java.io.OutputStream.class,
Socket.class, Process.class);
ctor.setAccessible(true);
return (JsonRpcClient) ctor.newInstance(blockingInput, sinkOutput, null, null);
}
Expand All @@ -76,18 +76,17 @@ void testTimeoutDoesNotFireAfterSessionClose() throws Exception {
try {
CopilotSession session = new CopilotSession("test-timeout-id", rpc);

CompletableFuture<AssistantMessageEvent> result = session.sendAndWait(
new MessageOptions().setPrompt("hello"), 2000);
CompletableFuture<AssistantMessageEvent> result = session
.sendAndWait(new MessageOptions().setPrompt("hello"), 2000);

assertFalse(result.isDone(), "Future should be pending before timeout fires");

// close() blocks up to 5s on session.destroy RPC. The 2s timeout
// fires during that window with the current per-call scheduler.
session.close();

assertFalse(result.isDone(),
"Future should not be completed by a timeout after session is closed. "
+ "The per-call ScheduledExecutorService leaked a TimeoutException.");
assertFalse(result.isDone(), "Future should not be completed by a timeout after session is closed. "
+ "The per-call ScheduledExecutorService leaked a TimeoutException.");
} finally {
rpc.close();
}
Expand All @@ -110,17 +109,17 @@ void testSendAndWaitReusesTimeoutThread() throws Exception {

long baselineCount = countTimeoutThreads();

CompletableFuture<AssistantMessageEvent> result1 = session.sendAndWait(
new MessageOptions().setPrompt("hello1"), 30000);
CompletableFuture<AssistantMessageEvent> result1 = session
.sendAndWait(new MessageOptions().setPrompt("hello1"), 30000);

Thread.sleep(100);
long afterFirst = countTimeoutThreads();
assertTrue(afterFirst >= baselineCount + 1,
"Expected at least one new sendAndWait-timeout thread after first call. "
+ "Baseline: " + baselineCount + ", after: " + afterFirst);
"Expected at least one new sendAndWait-timeout thread after first call. " + "Baseline: "
+ baselineCount + ", after: " + afterFirst);

CompletableFuture<AssistantMessageEvent> result2 = session.sendAndWait(
new MessageOptions().setPrompt("hello2"), 30000);
CompletableFuture<AssistantMessageEvent> result2 = session
.sendAndWait(new MessageOptions().setPrompt("hello2"), 30000);

Thread.sleep(100);
long afterSecond = countTimeoutThreads();
Expand All @@ -140,9 +139,7 @@ void testSendAndWaitReusesTimeoutThread() throws Exception {
* Counts the number of live threads whose name contains "sendAndWait-timeout".
*/
private long countTimeoutThreads() {
return Thread.getAllStackTraces().keySet().stream()
.filter(t -> t.getName().contains("sendAndWait-timeout"))
.filter(Thread::isAlive)
.count();
return Thread.getAllStackTraces().keySet().stream().filter(t -> t.getName().contains("sendAndWait-timeout"))
.filter(Thread::isAlive).count();
}
}
Loading