Skip to content

Add VirtualThreadPromiseFactory seed - #15960

Open
jamesfredley wants to merge 3 commits into
8.0.xfrom
feat/virtual-thread-promise-factory-seed
Open

Add VirtualThreadPromiseFactory seed#15960
jamesfredley wants to merge 3 commits into
8.0.xfrom
feat/virtual-thread-promise-factory-seed

Conversation

@jamesfredley

Copy link
Copy Markdown
Contributor

Description

What was found

Problem Impact
Async still centers on older factory options including GPars path Misses Java 21 virtual threads
Codebase 3 called for VirtualThreadPromiseFactory No implementation yet
GPars should be deprecation-track for 9.0 Needs docs + modern factory first

What changed

Area Change
Factory Add VirtualThreadPromiseFactory in grails-async-core
Selection Wire through PromiseFactoryBuilder with opt-in config
Docs Async guide notes GPars deprecation direction
Tests Virtual-thread factory coverage

Out of scope / follow-up

Topic Status
Removing grails-async-gpars 9.0
Full ThreadLocal audit Separate epic (#15828 partial)
Default factory flip Decision needed

Related MD topics

Source Topic
Codebase 3 GPars deprecation / VirtualThreadPromiseFactory
Google Doc 1.3 Virtual-thread readiness

Contributor Checklist

Issue and Scope

  • Starter modern async factory.
  • Opt-in, non-breaking default path preserved.
  • Single focused seed.
  • Targets 8.0.x.

Code Quality

  • Tests added.
  • Focused async-core tests run.
  • No mass reformatting.
  • AI starting point labeled.

Licensing and Attribution

  • Apache License 2.0.
  • Contributor rights confirmed.
  • ai-generated-starting-point label applied.

Documentation

  • Async docs updated.
  • Scope explained.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Introduce virtual-thread PromiseFactory and document GPars deprecation path.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 seeds Java 21 virtual-thread support in grails-async by introducing a new VirtualThreadPromiseFactory, wiring it into PromiseFactoryBuilder behind an opt-in system property, and documenting the intended deprecation direction for GPars.

Changes:

  • Added VirtualThreadPromiseFactory (virtual-thread-per-task executor) to grails-async-core.
  • Updated PromiseFactoryBuilder to select the virtual-thread factory when -Dgrails.async.promiseFactory=virtual-thread is set and no ServiceLoader factory is present.
  • Updated async guide documentation and added a new Spock spec covering selection/execution.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
grails-async/core/src/main/groovy/org/grails/async/factory/future/VirtualThreadPromiseFactory.groovy Adds the new virtual-thread-backed PromiseFactory implementation.
grails-async/core/src/main/groovy/org/grails/async/factory/PromiseFactoryBuilder.groovy Adds opt-in selection for the virtual-thread factory via system property.
grails-async/core/src/test/groovy/grails/async/VirtualThreadPromiseFactorySpec.groovy Adds tests for opt-in selection and basic execution behavior.
grails-doc/src/en/guide/async/asyncPromises.adoc Documents the opt-in virtual-thread factory and notes GPars deprecation direction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread grails-doc/src/en/guide/async/asyncPromises.adoc Outdated
@jdaugherty

Copy link
Copy Markdown
Contributor

We've previously discussed with @paulk-asert about integrating Gpars into groovy. I do not agree with it's deprecation.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.75758% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.3543%. Comparing base (e88612c) to head (69fbd65).
⚠️ Report is 6 commits behind head on 8.0.x.

Files with missing lines Patch % Lines
.../factory/future/VirtualThreadPromiseFactory.groovy 71.4286% 7 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #15960        +/-   ##
==================================================
+ Coverage     52.3514%   52.3543%   +0.0029%     
- Complexity      18299      18306         +7     
==================================================
  Files            2036       2037         +1     
  Lines           96347      96378        +31     
  Branches        16829      16831         +2     
==================================================
+ Hits            50439      50458        +19     
- Misses          38485      38495        +10     
- Partials         7423       7425         +2     
Files with missing lines Coverage Δ
.../grails/async/factory/PromiseFactoryBuilder.groovy 100.0000% <100.0000%> (ø)
.../factory/future/VirtualThreadPromiseFactory.groovy 71.4286% <71.4286%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jamesfredley

Copy link
Copy Markdown
Contributor Author

Thanks for the context on the GPars-into-Groovy discussions with @paulk-asert.

To separate the two things this PR touches: the deprecation of GPars and the modern virtual-thread path are independent decisions, and I'd propose decoupling them here.

  • We can leave GPars non-deprecated. This PR does not need to deprecate or remove grails-async-gpars / grails-events-gpars. Dropping the deprecation respects the ongoing Groovy/GPars integration discussion, and nothing in the virtual-thread seed depends on it.
  • What's worth landing is the modern path itself. A VirtualThreadPromiseFactory in grails-async-core is additive and opt-in - it gives applications on the JDK 21 baseline a maintained, native concurrency option alongside GPars, which is exactly the "virtual threads are the modern answer" direction in the pre-release review (Codebase 3 / guardrail: prioritize virtual threads, do not add a reactive/R2DBC stack).
  • Context, not a forcing function: GPars is at 1.2.1 (2014) and the upstream project is dormant, so the value of offering a virtual-thread-backed factory stands on its own regardless of GPars' lifecycle. I'll add a note to the PR capturing the GPars release age as motivation for the modern path, without implying an 8.x deprecation.

I'll rescope the PR to "add the VirtualThreadPromiseFactory seed" with GPars left as-is. Does that address the concern?

@jdaugherty jdaugherty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Other than the Gpars reference still, I'm ok with this PR

Comment thread grails-doc/src/en/guide/async/asyncPromises.adoc Outdated
@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
…on language

Addresses all 6 unresolved review comments (5 from Copilot, 1 from
jdaugherty), each verified before fixing:

- onComplete/onError constructed their FutureTaskPromise by casting the
  submitted closure to Closure<List<T>>, but the closure's actual
  returned value was callable.call(values) - typed T, not List<T>.
  Reproduced the resulting failure standalone outside the module first:
  a caller consuming the result as a statically-typed List<T> (the
  declared contract of Promise<List<T>>) hits a real
  GroovyCastException/ClassCastException at runtime, not just a
  theoretical mismatch. Fixed by resolving the promise to the actual
  waited-on List<T> (invoking the callback purely for its side effect,
  matching what "completion callback" implies) rather than the
  callback's own T-typed return value; onError now resolves to an empty
  list on failure instead of the same List<T>-typed lie. Added tests for
  onComplete/onError that consume the result as a statically-typed List
  to lock the fix in - these methods previously had zero test coverage.
- close() is now idempotent (guards on isShutdown()), matching
  CachedThreadPoolPromiseFactory's sibling implementation.
- @SInCE tags/doc version references corrected from 8.1 to 8.0, the
  actual branch/version this ships on.
- Test spec snapshots and restores the grails.async.promiseFactory
  system property and Promises.promiseFactory static field instead of
  unconditionally clearing them (parallel-fork test isolation), and
  closes the factory PromiseFactoryBuilder.build() constructs instead of
  leaking its ExecutorService.
- Removed the "GPars support is prepared for deprecation" doc language
  per jdaugherty's explicit objection (an active GPars/Groovy
  integration discussion is in progress with paulk-asert) and
  jamesfredley's agreed rescope: the virtual-thread factory is additive
  and opt-in, so it doesn't need to say anything about GPars' lifecycle.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 27, 2026
@testlens-app

testlens-app Bot commented Aug 6, 2026

Copy link
Copy Markdown

🚨 TestLens detected 1 failed test 🚨

Here is what you can do:

  1. Inspect the test failures carefully.
  2. If you are convinced that some of the tests are flaky, you can mute them below.
  3. Finally, trigger a rerun by checking the rerun checkbox.

Test Summary

CI / Functional Tests (Java 25, indy=false) > :grails-test-examples-gsp-sitemesh3:integrationTest

Test Runs Flakiness
EndToEndSpec > async multiple levels of layouts 1% 🟡

🏷️ Commit: 69fbd65
▶️ Tests: 55549 executed
⚪️ Checks: 60/60 completed

Test Failures

EndToEndSpec > async multiple levels of layouts (:grails-test-examples-gsp-sitemesh3:integrationTest in CI / Functional Tests (Java 25, indy=false))
Condition not satisfied:

pageSource == """<html><head><title>Decorated Base - Dialog - This is the title</title><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head> <body><h1>Hello</h1><div id="base"><div id="dialog">body text</div></div> </body></html>"""
|          |
|          false
|          Strings too large to calculate edit distance.
<html lang="en"><head><title>HTTP Status 500 – Internal Server Error</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2 {font-size:16px;} h3 {font-size:14px;} p {font-size:12px;} a {color:black;} .line {height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP Status 500 – Internal Server Error</h1></body></html>

	at EndToEndSpec.async multiple levels of layouts(EndToEndSpec.groovy:107)
expected actual
<html><head><title>Decorated Base - Dialog - This is the title</title><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head> <html lang="en"><head><title>HTTP Status 500 Internal Server Error</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2 {font-size:16px;} h3 {font-size:14px;} p {font-size:12px;} a {color:black;} .line {height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP Status 500 – Internal Server Error</h1></body></html>
<body><h1>Hello</h1><div id="base"><div id="dialog">body text</div></div>
</body></html>

Muted Tests

Select tests to mute in this pull request:

  • EndToEndSpec > async multiple levels of layouts

Reuse successful test results:

  • ♻️ Only rerun the tests that failed or were muted before

Click the checkbox to trigger a rerun:

  • Rerun jobs

Learn more about TestLens at testlens.app.

@jamesfredley

Copy link
Copy Markdown
Contributor Author

gpars like functionality will be in Groovy 6 for Grails 9: https://groovy-lang.org/releasenotes/groovy-6.0.html#concurrency-toolkit

Out of scope for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants