Skip to content

feat: Add automatic JDK 25 virtual thread support and simplify executor handling#227

Closed
brunoborges wants to merge 3 commits into
mainfrom
refactor/simplify-executor-handling
Closed

feat: Add automatic JDK 25 virtual thread support and simplify executor handling#227
brunoborges wants to merge 3 commits into
mainfrom
refactor/simplify-executor-handling

Conversation

@brunoborges
Copy link
Copy Markdown
Collaborator

This PR adds automatic virtual thread support on JDK 25+ and eliminates executor null-checking code smell throughout the SDK.

Summary

Addresses two code smell issues identified in executor handling:

  1. Redundant null checks in CopilotClient (lines 189, 330)
  2. Complex fallback logic in constructor (lines 159-163)

Changes

Multi-Release JAR for Virtual Threads

  • Added DefaultExecutorProvider with platform-specific implementations
  • Maven builds with JDK 25 to generate proper multi-release JAR
  • JAR manifest includes Multi-Release: true

Code Simplification

Eliminated redundant ternary null checks by ensuring executor is never null.

Documentation

  • Removed manual virtual thread instructions from README quick start
  • Updated requirements to explain automatic JDK 25+ virtual thread usage

Testing

  • Added DefaultExecutorProviderTest with multi-release JAR validation
  • Fixed JaCoCo to exclude multi-release classes from coverage
  • All 1005 tests pass

Benefits

  • Cleaner code without redundant null checks
  • Automatic virtual threads on JDK 25+ (no user action required)
  • Backward compatible with Java 17+
  • Proper lifecycle management

…or handling

This commit adds automatic virtual thread support on JDK 25+ via multi-release
JAR and eliminates executor null-checking code smell throughout the SDK.

**Key Changes:**

1. **Multi-Release JAR Support for Virtual Threads**
   - Added `DefaultExecutorProvider` with Java 17 base (returns ForkJoinPool.commonPool())
   - Added Java 25 multi-release version (returns virtual thread executor)
   - Updated Maven build to compile Java 25 version into META-INF/versions/25
   - Updated workflows to build with JDK 25 for proper multi-release JAR generation

2. **Eliminated Executor Null Checks**
   - Removed redundant ternary checks in CopilotClient (lines 189, 330)
   - Simplified constructor logic for executor initialization
   - Executor field is now guaranteed non-null

3. **Documentation Updates**
   - Removed manual virtual thread instructions from README quick start
   - Updated requirements to explain automatic JDK 25+ virtual thread usage
   - Simplified smoke test workflow (no longer modifies sample code)

4. **Test Coverage**
   - Added DefaultExecutorProviderTest with multi-release JAR validation
   - Fixed JaCoCo to exclude multi-release classes from coverage
   - All 1005 tests pass

**Benefits:**
- Cleaner, more maintainable code without null checks
- Automatic virtual thread adoption on JDK 25+ (no user action required)
- Backward compatible with Java 17+
- SDK manages executor lifecycle properly

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 26, 2026 17:05
Since executor is now guaranteed to be non-null, removed redundant
null checks on lines 442 and 530 before calling session.setExecutor().

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a multi-release JAR mechanism to automatically select a virtual-thread-based default executor on JDK 25+, while simplifying executor handling in CopilotClient and updating docs/CI to reflect the new default behavior.

Changes:

  • Add DefaultExecutorProvider with a JDK 25 multi-release implementation returning a virtual-thread-per-task executor.
  • Simplify CopilotClient to always use a non-null executor and introduce shutdown handling for SDK-owned executors.
  • Update Maven/CI configuration and documentation to build/publish with JDK 25 and document automatic virtual thread usage.
Show a summary per file
File Description
src/main/java/com/github/copilot/sdk/CopilotClient.java Centralizes executor selection and introduces executor lifecycle management.
src/main/java/com/github/copilot/sdk/DefaultExecutorProvider.java Provides the default executor implementation for non-JDK25 runtimes.
src/main/java25/com/github/copilot/sdk/DefaultExecutorProvider.java Multi-release JDK 25 implementation that uses virtual threads.
src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java Updates executor-related Javadoc to describe the new default strategy and ownership.
src/test/java/com/github/copilot/sdk/DefaultExecutorProviderTest.java Adds validation for multi-release behavior and virtual-thread execution on JDK 25.
pom.xml Adds a JDK 25 profile to compile multi-release classes and sets multi-release manifest entry; adjusts JaCoCo excludes.
README.md Updates requirements and removes manual “enable virtual threads” quick start instructions.
src/site/markdown/index.md Syncs documentation requirements with the new automatic virtual-thread default on JDK 25+.
.github/workflows/build-test.yml Runs the main CI test job on JDK 25 to exercise multi-release build output.
.github/workflows/run-smoke-test.yml Updates JDK 25 smoke-test instructions to rely on SDK defaults (no manual edits).
.github/workflows/publish-snapshot.yml Switches snapshot publishing to build with JDK 25.
.github/workflows/publish-maven.yml Switches release publishing to build with JDK 25.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 2

Comment on lines +161 to +163
this.ownedExecutor = providedExecutor == null && this.executor instanceof ExecutorService executorService
? executorService
: null;
Comment on lines +32 to +36
if (Runtime.version().feature() >= 25) {
return;
}

assertNull(DefaultExecutorProvider.create());
@edburns
Copy link
Copy Markdown
Collaborator

edburns commented May 26, 2026

Hi @brunoborges This PR is a surprise and was not a part of my migration plan https://github.com/github/copilot-sdk-partners/issues/80 . I'll try to get it in, but I must prioritize completing that plan and hitting the GA deadline.

Changed from utility class pattern to enum singleton pattern. This ensures
the executor is created once and reused across all CopilotClient instances,
which is especially beneficial for the virtual thread executor on JDK 25+.

Benefits:
- Single virtual thread executor shared across all clients (more efficient)
- Thread-safe lazy initialization (enum guarantee)
- Prevents instantiation without needing private constructor

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edburns
Copy link
Copy Markdown
Collaborator

edburns commented May 27, 2026

AI prompted risk assessment.

PR #227 Risk Assessment

Title: feat: Add automatic JDK 25 virtual thread support and simplify executor handling
Author: brunoborges (co-authored by Copilot)
Size: 13 files, +239 / -57
Created: 2026-05-26 (yesterday)


Commit Breakdown

# SHA Description Risk
1 bd7cdbc Add multi-release JAR, DefaultExecutorProvider, lifecycle mgmt, CI → JDK 25 HIGH
2 d761586 Remove executor null checks in createSession/resumeSession LOW
3 9d28aab Convert DefaultExecutorProvider to enum singleton + delete .vscode/mcp.json MEDIUM

High-Risk Issues

1. CI workflows now require JDK 25 to build

All four workflow files (build-test.yml, publish-maven.yml, publish-snapshot.yml, run-smoke-test.yml) change java-version: "17""25". This means:

  • No CI coverage on Java 17 — yet the SDK advertises Java 17 or later. If the multi-release JAR profile has any issue, Java 17 users discover it in production, not CI.
  • JDK 25 is a non-LTS early-access release. CI stability depends on distribution: "microsoft" having a reliable JDK 25 build available.

2. DefaultExecutorProvider as an enum singleton holds a process-lifetime executor

enum DefaultExecutorProvider {
    INSTANCE;
    private final Executor executor = Executors.newVirtualThreadPerTaskExecutor();
}

On JDK 25, this creates a single newVirtualThreadPerTaskExecutor() that lives for the entire JVM lifetime. This executor is never shut down (the ownedExecutor logic only applies when providedExecutor == null && executor instanceof ExecutorService). But since the enum holds it as a field of type Executor, the instanceof ExecutorService check in the constructor will match (because newVirtualThreadPerTaskExecutor() returns an ExecutorService). Wait — actually it's stored as Executor, but the instance check is on this.executor:

this.ownedExecutor = providedExecutor == null && this.executor instanceof ExecutorService executorService
        ? executorService : null;

This means every CopilotClient instance that doesn't provide a custom executor will call shutdown() on the shared singleton executor when closed. If you create a second client after closing the first, it will get a shut-down executor. This is a correctness bug.

3. Unrelated file deletion: .vscode/mcp.json

Commit 3 deletes .vscode/mcp.json (the MCP server config for gh aw). This is unrelated to executor handling and shouldn't be in this PR.

4. Behavioral change for existing users on Java 17

Previously, when no executor was set, CompletableFuture.supplyAsync(supplier) used ForkJoinPool.commonPool() implicitly. Now it explicitly passes ForkJoinPool.commonPool() from the enum. The behavior is equivalent on Java 17, but the ownedExecutor lifecycle logic will attempt to shutdown() the common pool — which is a no-op for ForkJoinPool.commonPool() (it ignores shutdown calls). So this is safe on Java 17 but only by accident.

5. Test only validates on JDK 25

DefaultExecutorProviderTest.multiReleaseProviderUsesVirtualThreadsOnJdk25() has a guard:

if (Runtime.version().feature() < 25) { return; }

Since CI only runs on JDK 25 now, the base-class test (testBaseImplementationReturnsForkJoinPool) will fail — it asserts assertEquals(ForkJoinPool.commonPool(), executor) but on JDK 25 the multi-release JAR will load the virtual-thread variant instead.

Wait — actually the test loads from target/classes which during the build would have the JDK 25 version at META-INF/versions/25/.... The base test calls DefaultExecutorProvider.INSTANCE.get() directly, which at runtime on JDK 25 would use the JDK 25 version from the multi-release class path... Actually no — at test time the classes are on the classpath directly (not in a JAR), so multi-release dispatch doesn't apply. The base test passes. But this means the base variant is never tested in a multi-release JAR context on JDK 25.


Medium-Risk Issues

Issue Detail
No Java 17 test matrix SDK promises Java 17+ but CI only builds/tests on 25
Smoke test simplification Removed the JDK 25 virtual thread setup instructions from the smoke test, relying entirely on multi-release JAR working correctly — which is untested on 17 in CI
README simplification Quick start no longer shows how to manually configure an executor, reducing user control visibility

Low-Risk / Positive

  • Commits 2 & 3 remove legitimate code smell (null-check ternaries)
  • The multi-release JAR approach is architecturally sound in principle
  • Javadoc updates in CopilotClientOptions are accurate and helpful
  • JaCoCo exclusion for META-INF/versions/** is correct

Verdict

Recommend: DO NOT MERGE as-is before GA. Key reasons:

  1. Potential correctness bug: Shared singleton executor gets shut down by the first client that closes, breaking subsequent clients.
  2. CI no longer tests on Java 17, the minimum supported version.
  3. Unrelated change (.vscode/mcp.json deletion) bundled in.
  4. Insufficient test coverage for the multi-release behavior on the actual target (Java 17 running without virtual threads).

If the feature is desired for GA, it needs at minimum:

  • Fix the shared-executor-shutdown bug (don't assign ownedExecutor when using the singleton)
  • Restore Java 17 in the CI test matrix (or add a matrix entry)
  • Remove the unrelated .vscode/mcp.json deletion
  • Add a Java 17 integration test that proves the non-virtual-thread path works in a packaged JAR

@edburns
Copy link
Copy Markdown
Collaborator

edburns commented May 27, 2026

I agree with the observations in the risk assessment.

Also, the PR must be reworked to take into account the drift from main since the PR was opened.

For these reasons, and the directive from @patniko that we need to have all the changes in by COB PDT 2026-05-29, I judge this can safely be deferred until after GA.

@edburns
Copy link
Copy Markdown
Collaborator

edburns commented May 27, 2026

Please re-open or create a new PR after GA.

@edburns edburns closed this May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants