@edburns should we throw an IAE if the user passes null?

@brunoborges wrote:

should we throw an IAE if the user passes null?

We don't do that for other setters in CopilotClientOptions. I might say "yes" we should do that, but I'd like to understand why we would not do it for all other options before I fully commit to "yes" in the case of Executor.

@brunoborges wrote:

should we throw an IAE if the user passes null?

We don't do that for other setters in CopilotClientOptions. I might say "yes" we should do that, but I'd like to understand why we would not do it for all other options before I fully commit to "yes" in the case of Executor.

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:

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.

Of course. I'll take this as consent to add the null checks throughout, at least in the CopilotClientOptions class.

I'll take the remainder of the null checks as a separate issue.

@brunoborges wrote:

There was no deep thinking before. Making null-safe now leads us to a more stable and strict API, and towards GA.
...

Done in commit 9098d5a. All reference-type setters in CopilotClientOptions now call Objects.requireNonNull and throw NullPointerException for null arguments: setCliArgs, setCliPath, setCliUrl, setCwd, setEnvironment, setExecutor, setGitHubToken, setGithubToken (deprecated), setLogLevel, setOnListModels, and setTelemetry. Javadoc "or null to…" wording removed from affected setters.

The one test that explicitly called setCliPath(null) (CliServerManagerTest.startCliServerWithNullCliPath) was updated to use a default CopilotClientOptions() without calling setCliPath — which is the correct migration pattern: simply don't call the setter to get the default (null) field value.

Proof of sufficiency for Virtual Threads implementation.

17:11 Prompt

Looking overall at the potential improvements by taking JDK 21 as a minimum JDK level, and using virtual threads if they are present, is the API change exemplified in

try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
    var options = new CopilotClientOptions().setExecutor(executor);
    try (CopilotClient client = new CopilotClient(options)) {
        client.start().get();
        // SDK internals run on virtual threads (no carrier pinning)
        // Caller .get() calls on virtual threads also don't pin
        var session = client.createSession(config).get();
        session.sendAndWait(new MessageOptions().setPrompt("What is 2+2?")).get();
    }
}

sufficient for allowing users to get the most of virtual threads when using the public API of this codebase?

What other changes to the public API might be necessary in light of the ability to use virtual threads?

18:33 Response

Let me examine the public API surface to give you a thorough answer.

Now I have a thorough picture. Here's my analysis:

Yes, setExecutor() is the right and sufficient public API change. The virtual-thread example you showed is exactly the intended usage. But there are three internal areas that would benefit from further work if you raised the minimum to JDK 21 or detect virtual threads at runtime:

1. synchronized → ReentrantLock (carrier-thread pinning)

Virtual threads pin their carrier when inside a synchronized block. The SDK has these synchronized sites:

The sendMessage() one matters. Converting it from synchronized to ReentrantLock would allow virtual threads to unmount while waiting for the lock. No public API change needed — this is purely internal. JDK 21's synchronized pinning fix is scheduled for a later release (JEP 491 landed in JDK 24), so for JDK 21 this is a real concern.

2. Event handlers (on()) run on the reader thread

dispatchEvent() calls Consumer<AbstractSessionEvent> handlers synchronously on the jsonrpc-reader daemon thread. If a handler blocks (e.g., does I/O), it stalls all event processing. This isn't a virtual-thread issue per se, but if the SDK executor is virtual-thread-based, you might expect event dispatch to also go through it.

Possible API addition (not strictly necessary):

// Today: events dispatched synchronously on reader thread
session.on(AssistantMessageEvent.class, msg -> { ... });

// Possible: events dispatched through session executor
session.onAsync(AssistantMessageEvent.class, msg -> { ... });

However, this would change ordering semantics (events could arrive out-of-order if dispatched async). Probably leave this alone — the current design is consistent with the upstream .NET SDK.

3. The two internal daemon threads don't go through the executor

• jsonrpc-reader (in JsonRpcClient) — platform thread, reads stdio
• sendAndWait-timeout (in CopilotSession) — ScheduledThreadPoolExecutor

Neither uses the user-provided executor. This is actually correct by design:

• The reader thread does blocking I/O on ProcessInputStream — making it virtual wouldn't help (it's pinned on a native read syscall anyway).
• The timeout scheduler must be a ScheduledExecutorService; Executor doesn't support scheduled tasks, and virtual threads don't improve timer-wheel behaviour.

No API change needed here.

Summary

The public API is sufficient. The one thing worth doing internally — if you target JDK 21 or want to be virtual-thread-friendly on JDK 21-23 — is replacing synchronized in JsonRpcClient.sendMessage() with a ReentrantLock. That's a one-line internal change with no public API impact.

Want to tackle the sendMessage() lock conversion?

18:37 Prompt

Maybe. But first help me understand. If someone takes our JDK 17 compiled library and runs it on JDK 25, which has JEP 491, is the synchronized thing already addressed by virtue of JEP 491?

18:40 Response

Yes. JEP 491 landed in JDK 24 (March 2025) and is a JVM runtime change, not a bytecode change. The monitorenter/monitorexit instructions emitted by javac --release 17 are identical — it's the JVM that now knows how to unmount a virtual thread while it waits to acquire a monitor, instead of pinning it to the carrier.

So: your JDK 17-compiled JAR running on JDK 24+ (including 25) automatically gets unpinned synchronized with zero code changes.

The ReentrantLock conversion would only matter for users running on JDK 21, 22, or 23 with virtual threads. Given that JDK 21 is the only LTS in that range and JDK 25 (the next LTS) includes JEP 491, the window where ReentrantLock helps is narrow and shrinking.

Probably not worth the churn.

18:41 Prompt

Next question, so let's say the user takes our JDK 17 compiled jar and runs it on JDK 25. Do they automatically get the Virtual Thread goodness, or do we need to make API changes and release a special Java 25 version of the jar to let that goodness flow through to users?

18:43 Response

They don't get it automatically. Two reasons:

1. ForkJoinPool.commonPool() is still platform threads on JDK 25. The JVM doesn't retroactively switch the common pool to virtual threads. CompletableFuture.runAsync(task) without an explicit executor still runs on platform threads.

2. Virtual threads are opt-in. You only get them by explicitly calling Executors.newVirtualThreadPerTaskExecutor() or Thread.ofVirtual().

So on JDK 25, users still need this one line to unlock virtual threads:

var options = new CopilotClientOptions()
    .setExecutor(Executors.newVirtualThreadPerTaskExecutor());

No special JDK 25 build of the jar is required — our JDK 17-compiled jar accepts any Executor, and newVirtualThreadPerTaskExecutor() returns a standard ExecutorService. It just works.

The alternative would be to auto-detect virtual thread support at runtime and use it as the default:

// Hypothetical: in CopilotClient, when executor is null
private static Executor defaultExecutor() {
    try {
        return (ExecutorService) Executors.class
            .getMethod("newVirtualThreadPerTaskExecutor")
            .invoke(null);
    } catch (ReflectiveOperationException e) {
        return ForkJoinPool.commonPool(); // JDK 17-20 fallback
    }
}

But that changes default behavior invisibly, which could surprise users (virtual threads have different ThreadLocal and stack-trace characteristics). The explicit setExecutor() opt-in is the safer design — one line of user code, no special jar.

Full prompt files for this work.

01-mock-llm-to-show-blocking.md
02-accept-executor.md
03-accept-executor.md
04-copilot-client-options.md