feat(java): JDK 25 default virtual-thread executor via multi-release JAR#1478
Closed
brunoborges wants to merge 15 commits into
Closed
feat(java): JDK 25 default virtual-thread executor via multi-release JAR#1478brunoborges wants to merge 15 commits into
brunoborges wants to merge 15 commits into
Conversation
Use a multi-release JAR to select virtual threads as the default internal executor on JDK 25+, while retaining the common pool on older JDKs. Keep user-provided executors caller-owned and only shut down SDK-owned defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nership and shutdown capability
…executor management
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a multi-release JAR mechanism so the SDK selects virtual threads on JDK 25+ and ForkJoinPool.commonPool() on older JDKs as its default internal executor, while keeping user-provided executors untouched. Centralizes executor handling in CopilotClient and ensures the SDK shuts down only executors it owns.
Changes:
- Introduces
InternalExecutorProvider(base + JDK 25 override) and routes allCopilotClientasync work through it, shutting down owned executors onclose()/forceStop(). - Updates the Maven build with a
java25-multi-releaseprofile and a Failsafe-based integration test (InternalExecutorProviderIT+InternalExecutorProviderProbe) that exercises the packaged JAR. - Updates Javadoc/README to reflect the new default-executor semantics.
Show a summary per file
| File | Description |
|---|---|
| java/src/main/java/com/github/copilot/InternalExecutorProvider.java | Base provider using ForkJoinPool.commonPool(); not owned. |
| java/src/main/java25/com/github/copilot/InternalExecutorProvider.java | JDK 25 override using virtual-thread-per-task executor; owned. |
| java/src/main/java/com/github/copilot/CopilotClient.java | Uses provider, simplifies async dispatch, shuts down owned executor on close/forceStop. |
| java/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java | Doc updates for new default executor semantics. |
| java/src/test/java/com/github/copilot/InternalExecutorProviderTest.java | Unit tests for base provider and that user executors aren't shut down. |
| java/src/test/java/com/github/copilot/InternalExecutorProviderProbe.java | Helper main spawned by IT to probe runtime behavior. |
| java/src/test/java/com/github/copilot/InternalExecutorProviderIT.java | Failsafe IT validating MR-JAR behavior against packaged JAR. |
| java/pom.xml | Adds Failsafe plugin, jacoco MR-class exclude, and java25-multi-release profile. |
| java/README.md | Documents auto-virtual-thread default on JDK 25+ and simplifies quick start. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 5
…pdate documentation
…rtion tests Addresses PR github#1478 review (Copilot AI): the existing baseProviderUsesCommonPoolWithoutOwnership method bundled three unrelated assertions (commonPool identity, user-executor ownership, package-private visibility). Split into baseProviderReturnsCommonPool, userProvidedExecutorIsNotOwned, and providerIsPackagePrivate so failures point at a single condition each.
…Stop() Addresses PR github#1478 review (Copilot AI, discussion r3314987809): CopilotClient#forceStop() chains shutdownOwnedExecutor() onto cleanupConnection() via whenComplete(...), but cleanupConnection() is itself built on async work scheduled on the SDK-owned executor (e.g. CompletableFuture.supplyAsync(..., executor) in connection setup). On JDK 25+ this means the whenComplete lambda can land on one of the owned executor's threads; awaitTermination(...) then blocks waiting for the very thread it is running on, forcing the full AUTOCLOSEABLE_TIMEOUT_SECONDS timeout followed by shutdownNow(). Fix: dispatch the shutdown continuation via whenCompleteAsync(...) onto a private one-shot SHUTDOWN_DISPATCHER that spawns a fresh daemon thread named "copilot-client-shutdown". This guarantees the awaitTermination call is never made from inside the executor it is draining. close() is unaffected: it calls stop().get(...) synchronously and runs shutdownOwnedExecutor() in its finally block on the caller's thread.
Addresses PR github#1478 review (Copilot AI, discussion r3314987870): close() and forceStop() can each invoke shutdownOwnedExecutor() (e.g. user calls forceStop() and then close() in try-with-resources). A second call would redundantly invoke shutdown() and awaitTermination() on an already- terminated ExecutorService. While the JDK handles this gracefully (awaitTermination returns immediately after a prior shutdownNow), the redundant call obscures diagnostics. Short-circuit at the top of shutdownOwnedExecutor() when isShutdown() is already true and log at FINE so the second invocation is visible without spamming normal output.
…DK tests across JDK 17 and 25
The java25-multi-release Maven profile is activated only on JDK 25+
(<jdk>[25,)</jdk>). Without it, the build skips the compile-java25 execution,
the packaged JAR has no META-INF/versions/25/InternalExecutorProvider.class,
and the manifest lacks Multi-Release: true. The InternalExecutorProvider
JDK 25 overlay (Executors.newVirtualThreadPerTaskExecutor()) is then
effectively dead in CI and in published Maven Central artifacts -- consumers
on JDK 25+ silently fall back to ForkJoinPool.commonPool().
Changes:
- java-publish-snapshot.yml: set up JDK 25 (was 17). The pom keeps
<maven.compiler.release>17</maven.compiler.release> so baseline bytecode
remains JDK 17 compatible; --release 17 is supported by the JDK 25
compiler.
- java-publish-maven.yml: same JDK bump for release:perform.
- java-sdk-tests.yml: matrix on java-version: [17, 25]. JDK 25 entry
exercises the MR-JAR overlay end-to-end via InternalExecutorProviderIT
(asserts feature >= 25 => canBeShutdown=true, virtual=true) and runs the
new verify-java25-overlay antrun structural guard. Side-effects (site
artifact upload, JaCoCo badge generation, badge-update PR) remain gated to
the JDK 17 entry so the badge source-of-truth stays a single baseline.
Failure artifact name suffixed with -jdk${matrix.java-version} to avoid
collisions.
Branch protection note: the job's check name changes from "Java SDK Tests"
to "Java SDK Tests (JDK 17)" + "Java SDK Tests (JDK 25)". Update branch
protection rules accordingly after merge if required-checks reference the
old name.
edburns
added a commit
that referenced
this pull request
May 28, 2026
Cherry-pick code changes from brunoborges/java-jdk25-default-executor: - Instance-based InternalExecutorProvider with canBeShutdown() API - SHUTDOWN_DISPATCHER to avoid deadlock in forceStop() - Idempotent shutdownOwnedExecutor() (short-circuits if already shut down) - maven-failsafe-plugin + InternalExecutorProviderIT (spawns child JVM against packaged JAR) - InternalExecutorProviderProbe for IT verification - Antrun verify-java25-overlay (checks actual JAR zip entry in java25-multi-release profile) - Simplified README (no manual executor configuration needed) - Split unit tests into single-assertion methods Co-authored-by: Bruno Borges <bruno@brunoborges.com>
Collaborator
|
Copied all commits to #1483 . Iterating there to push directly to upstream for CI/CD updates. @brunoborges please push additional commits if necessary to that PR. |
edburns
pushed a commit
that referenced
this pull request
May 28, 2026
…rtion tests Addresses PR #1478 review (Copilot AI): the existing baseProviderUsesCommonPoolWithoutOwnership method bundled three unrelated assertions (commonPool identity, user-executor ownership, package-private visibility). Split into baseProviderReturnsCommonPool, userProvidedExecutorIsNotOwned, and providerIsPackagePrivate so failures point at a single condition each.
edburns
pushed a commit
that referenced
this pull request
May 28, 2026
…Stop() Addresses PR #1478 review (Copilot AI, discussion r3314987809): CopilotClient#forceStop() chains shutdownOwnedExecutor() onto cleanupConnection() via whenComplete(...), but cleanupConnection() is itself built on async work scheduled on the SDK-owned executor (e.g. CompletableFuture.supplyAsync(..., executor) in connection setup). On JDK 25+ this means the whenComplete lambda can land on one of the owned executor's threads; awaitTermination(...) then blocks waiting for the very thread it is running on, forcing the full AUTOCLOSEABLE_TIMEOUT_SECONDS timeout followed by shutdownNow(). Fix: dispatch the shutdown continuation via whenCompleteAsync(...) onto a private one-shot SHUTDOWN_DISPATCHER that spawns a fresh daemon thread named "copilot-client-shutdown". This guarantees the awaitTermination call is never made from inside the executor it is draining. close() is unaffected: it calls stop().get(...) synchronously and runs shutdownOwnedExecutor() in its finally block on the caller's thread.
edburns
pushed a commit
that referenced
this pull request
May 28, 2026
Addresses PR #1478 review (Copilot AI, discussion r3314987870): close() and forceStop() can each invoke shutdownOwnedExecutor() (e.g. user calls forceStop() and then close() in try-with-resources). A second call would redundantly invoke shutdown() and awaitTermination() on an already- terminated ExecutorService. While the JDK handles this gracefully (awaitTermination returns immediately after a prior shutdownNow), the redundant call obscures diagnostics. Short-circuit at the top of shutdownOwnedExecutor() when isShutdown() is already true and log at FINE so the second invocation is visible without spamming normal output.
edburns
added a commit
that referenced
this pull request
May 28, 2026
Bruno authored the core code with Copilot. * feat(java): add JDK 25 default executor Use a multi-release JAR to select virtual threads as the default internal executor on JDK 25+, while retaining the common pool on older JDKs. Keep user-provided executors caller-owned and only shut down SDK-owned defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(java): cover owned default executor shutdown Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(java): make default executor provider internal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(java): update InternalExecutorProvider to manage executor ownership and shutdown capability * feat(java): add integration tests for multi-release JAR behavior and executor management * feat(java): add JDK 25 multi-release overlay class verification and update documentation * test(java): split InternalExecutorProvider unit test into single-assertion tests Addresses PR #1478 review (Copilot AI): the existing baseProviderUsesCommonPoolWithoutOwnership method bundled three unrelated assertions (commonPool identity, user-executor ownership, package-private visibility). Split into baseProviderReturnsCommonPool, userProvidedExecutorIsNotOwned, and providerIsPackagePrivate so failures point at a single condition each. * fix(java): dispatch owned-executor shutdown off the executor in forceStop() Addresses PR #1478 review (Copilot AI, discussion r3314987809): CopilotClient#forceStop() chains shutdownOwnedExecutor() onto cleanupConnection() via whenComplete(...), but cleanupConnection() is itself built on async work scheduled on the SDK-owned executor (e.g. CompletableFuture.supplyAsync(..., executor) in connection setup). On JDK 25+ this means the whenComplete lambda can land on one of the owned executor's threads; awaitTermination(...) then blocks waiting for the very thread it is running on, forcing the full AUTOCLOSEABLE_TIMEOUT_SECONDS timeout followed by shutdownNow(). Fix: dispatch the shutdown continuation via whenCompleteAsync(...) onto a private one-shot SHUTDOWN_DISPATCHER that spawns a fresh daemon thread named "copilot-client-shutdown". This guarantees the awaitTermination call is never made from inside the executor it is draining. close() is unaffected: it calls stop().get(...) synchronously and runs shutdownOwnedExecutor() in its finally block on the caller's thread. * fix(java): short-circuit shutdownOwnedExecutor() when already shut down Addresses PR #1478 review (Copilot AI, discussion r3314987870): close() and forceStop() can each invoke shutdownOwnedExecutor() (e.g. user calls forceStop() and then close() in try-with-resources). A second call would redundantly invoke shutdown() and awaitTermination() on an already- terminated ExecutorService. While the JDK handles this gracefully (awaitTermination returns immediately after a prior shutdownNow), the redundant call obscures diagnostics. Short-circuit at the top of shutdownOwnedExecutor() when isShutdown() is already true and log at FINE so the second invocation is visible without spamming normal output. * spotless:apply * ci(java): build/publish on JDK 25 to include MR-JAR overlay; matrix SDK tests across JDK 17 and 25 The java25-multi-release Maven profile is activated only on JDK 25+ (<jdk>[25,)</jdk>). Without it, the build skips the compile-java25 execution, the packaged JAR has no META-INF/versions/25/InternalExecutorProvider.class, and the manifest lacks Multi-Release: true. The InternalExecutorProvider JDK 25 overlay (Executors.newVirtualThreadPerTaskExecutor()) is then effectively dead in CI and in published Maven Central artifacts -- consumers on JDK 25+ silently fall back to ForkJoinPool.commonPool(). Changes: - java-publish-snapshot.yml: set up JDK 25 (was 17). The pom keeps <maven.compiler.release>17</maven.compiler.release> so baseline bytecode remains JDK 17 compatible; --release 17 is supported by the JDK 25 compiler. - java-publish-maven.yml: same JDK bump for release:perform. - java-sdk-tests.yml: matrix on java-version: [17, 25]. JDK 25 entry exercises the MR-JAR overlay end-to-end via InternalExecutorProviderIT (asserts feature >= 25 => canBeShutdown=true, virtual=true) and runs the new verify-java25-overlay antrun structural guard. Side-effects (site artifact upload, JaCoCo badge generation, badge-update PR) remain gated to the JDK 17 entry so the badge source-of-truth stays a single baseline. Failure artifact name suffixed with -jdk${matrix.java-version} to avoid collisions. Branch protection note: the job's check name changes from "Java SDK Tests" to "Java SDK Tests (JDK 17)" + "Java SDK Tests (JDK 25)". Update branch protection rules accordingly after merge if required-checks reference the old name. * On branch edburns/review-brunoborges-pr-1478 Test invocation changes. modified: .github/workflows/java-sdk-tests.yml - Have `test-jdk17` be a parameter, set to true by default. - Do not use matrix. It is vital that the tests verify that the MR-JAR facility works when compiled using JDK 25 with `maven.compiler.release` set to 17. To that end, this workflow does not re-compile the jar or the tests, but uses a the JDK 17 just to run the tests. - Use a separate banner for each variant. - Use separate summary for 17 and 25 tests. modified: java/pom.xml - Print banners for inspection during tests. - Use Maven enforcer plugin to require using 25 when compiling. modified: java/README.md - Add content for running the tests with 17. * On branch edburns/review-brunoborges-pr-1478 modified: .github/workflows/docs-validation.yml - Use Java 25 per @brunoborges. modified: .github/workflows/java-sdk-tests.yml - Update labels. modified: .vscode/settings.json - Additional Java settings. modified: scripts/docs-validation/package.json The `--lang` parsing in validate.ts uses `args.find((a) => a.startsWith("--lang="))`, which won't match `--lang typescript` (space-separated). So `targetLang` is always `undefined`, and every job validates **all** languages. This is a pre-existing bug — but it was invisible before because `mvn install` succeeded with the pre-installed JDK 17 on all runners. However, the reason it matters **now** is: after fixing the `validate-java` job to use JDK 25, the other 4 jobs (TypeScript, Go, Python, C#) still don't have JDK 25. Since they also accidentally validate Java (due to the broken `--lang` filter), they'd continue to fail. So you actually have two options: **Option A:** Only fix JDK in `validate-java` AND fix `--lang` parsing so other jobs stop accidentally validating Java. **Option B:** Only fix JDK in `validate-java` AND add JDK 25 setup to all 4 other jobs too (ugly but works without touching the script). The `--lang` fix is the cleaner path, but it's a separate pre-existing bug, not something introduced by PR #1483. If you'd prefer to keep the changes minimal and just address the PR's breakage, I can revert the package.json change and instead add `setup-java` with JDK 25 to every job. What's your preference? * On branch edburns/review-brunoborges-pr-1478 modified: .github/actions/java-test-report/action.yml modified: .github/workflows/java-sdk-tests.yml - Ensure the correct Copilot CLI is used for tests. * On branch edburns/review-brunoborges-pr-1478 modified: .github/workflows/java-sdk-tests.yml Now the JDK 17 run: 1. `jacoco:prepare-agent@wire-up-coverage-instrumentation` — generates the JaCoCo agent arg compatible with JDK 17 and sets `testExecutionAgentArgs` 2. `antrun:run@print-test-jdk-banner` — prints the JDK banner 3. `surefire:test` — runs pre-compiled tests with the JaCoCo agent attached 4. `jacoco:report@build-coverage-report-from-tests` — generates the HTML/XML/CSV reports The `-DtestExecutionAgentArgs=` override is removed so JaCoCo's prepared value flows through to Surefire's `<argLine>`. The test report action already reads from the default jacoco.xml path, so the coverage section will appear in both reports automatically. * On branch edburns/review-brunoborges-pr-1478 Address copilot review comments. modified: .github/actions/java-test-report/action.yml - Add failsafe tests to the report. modified: .github/workflows/java-sdk-tests.yml - Ensure failsafe tests are invoked in the 17 case. modified: java/README.md - Fix spelling error. - Fix artifact version error. - Fix 17 invocation. * Ensure the Jar is produced --------- Co-authored-by: Bruno Borges <brborges@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Superseded by #1483 .
Summary
Adds a JDK 25-optimized default executor to the Java SDK via a multi-release JAR, refactors
CopilotClientto centralize executor resolution and ownership, and introduces a Failsafe-based integration test that validates the multi-release JAR end-to-end against the actually-packaged artifact.When a caller does not supply an
ExecutorviaCopilotClientOptions.setExecutor(...), the SDK now picks the best default for the runtime:close()ForkJoinPool.commonPool()Executors.newVirtualThreadPerTaskExecutor()User-provided executors are always treated as un-owned and are never shut down by the SDK.
Changes
Runtime (
src/main/java/...)InternalExecutorProvider(package-private) — single source of truth for executor selection. Constructed with the user-providedExecutor(may benull); exposesget()andcanBeShutdown().ForkJoinPool.commonPool();canBeShutdown()is alwaysfalse.CopilotClientInternalExecutorProviderand cachesexecutor+executorCanBeShutdownas final fields.CompletableFuture.supplyAsync/runAsynccall sites and theRpcHandlerDispatcher/CopilotSessionwiring now use this single field instead of repeatedly callingoptions.getExecutor()and branching onnull.shutdownOwnedExecutor()runs on bothclose()(infinally) andforceStop()(chained offcleanupConnection()). It callsshutdown(), waits up toAUTOCLOSEABLE_TIMEOUT_SECONDS, falls back toshutdownNow(), and correctly re-interrupts onInterruptedException. Skips entirely when the SDK does not own the executor.CopilotClientOptions— Javadoc ongetExecutor()/setExecutor()updated to:JDK 25 overlay (
src/main/java25/...)InternalExecutorProvider(JDK 25 variant) — when no user executor is supplied, instantiatesExecutors.newVirtualThreadPerTaskExecutor()and reportscanBeShutdown() == true. When a user executor is supplied, behaves identically to the base provider (un-owned).--release 25intoMETA-INF/versions/25/via a newjava25-multi-releaseMaven profile (activation<jdk>[25,)</jdk>).Build (
pom.xml)java25-multi-releaseprofile — JDK-activated. Adds acompile-java25execution tomaven-compiler-pluginwith<release>25</release>,<multiReleaseOutput>true</multiReleaseOutput>, andcompileSourceRoot = src/main/java25. Configuresmaven-jar-pluginto addMulti-Release: trueto the manifest.maven-failsafe-plugin3.5.5 — new plugin block bound tointegration-test+verify. Exposesproject.build.directory,project.build.finalName, andproject.build.testOutputDirectoryas system properties so the IT can locate the packaged JAR.META-INF/versions/**/*.classto avoid duplicate-class analysis on the MR overlay.Tests (
src/test/java/...)InternalExecutorProviderTestbaseProviderUsesCommonPoolWithoutOwnership— verifies the no-user-executor path returnsForkJoinPool.commonPool()andcanBeShutdown() == falseon whatever JDK runs the unit-test phase.clientDoesNotShutDownUserProvidedExecutor— passes a customExecutorServicetoCopilotClient, closes the client, asserts the executor is still alive.InternalExecutorProviderIT(Failsafe)target/${finalName}.jar) +target/test-classes, runsInternalExecutorProviderProbe, parseskey=valuestdout.feature >= 25⇒canBeShutdown=trueandvirtual=true; otherwise bothfalse.InternalExecutorProviderProbe— tinymainin the same package asInternalExecutorProvider. Constructs the provider withnull, submits a task, reflectively callsThread.isVirtual()(for JDK 17 source-level compatibility), printsfeature=,canBeShutdown=,virtual=, and exits non-zero on task timeout.Docs
README.mdtouch-up reflecting the executor behavior.Why this approach for testing MR-JAR behavior
Earlier iterations of this branch tested the JDK 25 overlay via reflection on
CopilotClientinternals and synthesized JARs overtarget/classes. Both were fragile and coupled tests to private fields. The Failsafe IT instead exercises the actual packaged artifact in a child JVM, which is the same code path real consumers run, with no reflection on SDK internals and no split-package issues (same-package probe runs on the classpath as the unnamed module).Validation
./mvnw verify— BUILD SUCCESS in ~4 min on JDK 25:default-test: 1002 passed, 4 skipped, 0 failuresisolated-resume-tests: 3 passedjar:jar: buildstarget/copilot-sdk-java-1.0.0-beta-java.5-SNAPSHOT.jarwithMETA-INF/versions/25/overlay andMulti-Release: truemanifest entryintegration-test(InternalExecutorProviderIT): 1 passed — child JVM reportsfeature=25, canBeShutdown=true, virtual=trueverify: no failuresCompatibility
--release 17; no source/binary changes for users on JDK 17–24.CopilotClientOptions.getExecutor()still returnsnullwhen unset; the new "virtual-threads on JDK 25+" behavior is purely internal.close().