matrei commented on PR #16230:
URL: https://github.com/apache/grails-core/pull/16230#issuecomment-5709736273
## Review round 2
**Head:** `b65a64ed69` (`feature/modernize-grails-async-8.0.x`, 18
first-parent commits) · **Base:** `8.0.x` @ `a5758d657f`. The head merges
upstream `8.0.x` up to the #15960 merge (`VirtualThreadPromiseFactory`); the
two `8.0.x` merges after that (#16322, #16184) are not included and
`upgrading80x.adoc` conflicts with them (N9).
Six commits since round 1: five fix commits (`f269620071` … `a29efe7561`)
reviewed in detail below, plus the upstream merge `b65a64ed69`, covered in the
post-merge addendum. The "Verified as correct" list from round 1 still holds.
**What I ran locally** (on the merged head `b65a64ed69`; the N1/N2 probes
were run on `a29efe7561`, and none of the files they exercise changed in the
merge)
| Check | Result |
|---|---|
| `:grails-async-core:cleanTest test` (`--no-build-cache`) | 69 tests, 0
failures, 1 skipped |
| `:grails-async:cleanTest test` (`--no-build-cache`) | 27 tests, 0 failures
|
| `:grails-events-core:cleanTest test` (`--no-build-cache`) | 12 tests, 0
failures |
| `:grails-async-core:codeStyle :grails-async:codeStyle
:grails-events-core:codeStyle` | pass |
| `git merge-tree origin/8.0.x pr/16230` | conflict in `upgrading80x.adoc`
only |
| Throwaway probe spec (deleted afterwards) for N1 and N2 below | output
quoted per finding |
**Verdict: request changes, but close.** All four High findings from round 1
are fixed and covered by tests (see "Round 1 status" at the end). The rework of
the exception path exposed one new regression that the new tests miss because
they only throw unchecked exceptions, and the standalone `WebPromises` fix
restored propagation for the task closure but not for `then {}` callbacks,
which the 7.x implementation did propagate.
---
### High
**N1. Checked exceptions thrown from a task reach `get()`/`onError` wrapped
in `UndeclaredThrowableException`.**
`CompletableFuturePromiseFactory.groovy:78` submits `{ decorated.call() } as
Supplier<T>`. Under `@CompileStatic` the `as` coercion still produces a
`java.lang.reflect.Proxy` over `ConvertedClosure`; `Supplier.get()` declares no
checked exceptions, so the proxy wraps any checked exception in
`UndeclaredThrowableException` before `CompletableFuture` ever sees it.
`unwrap()` (`CompletableFuturePromise.groovy:109`) only strips
`CompletionException`/`ExecutionException`, so the wrapper survives. The same
coercion sits in `asCompletableFuture`
(`CompletableFuturePromiseFactory.groovy:144`, where `promise.get()` itself
throws checked `ExecutionException`) and in the new standalone web decorator
(`WebPromises.groovy:71`, `{ … } as Runnable`).
The old `FutureTask` implementation ran a `Callable`, so `task { throw new
IOException() }.get()` used to throw `ExecutionException(IOException)` and
`onError` received the `IOException`. Groovy code throws checked exceptions
without declaring them all the time, so this is the common case, not an edge
case, and it breaks `exception:` URL-mapping matches and `onError` type checks
for every `IOException`/`SQLException`/`TimeoutException` coming out of a task.
The new `CompletableFuturePromiseFactorySpec` cases all throw
`IllegalStateException`, which is why they pass.
*Probe (standalone factory, `Promises.task { throw new IOException('io')
}`):*
```
direct get(): ExecutionException -> UndeclaredThrowableException ->
IOException
onError sees: UndeclaredThrowableException (cause IOException)
WebPromises: UndeclaredThrowableException (cause IOException)
```
*Fix:* never let a closure cross a proxy boundary. Either catch `Throwable`
inside the coerced closure and complete the future yourself (`new
CompletableFuturePromise(executor)` + `executor.execute { try {
promise.complete(decorated.call()) } catch (Throwable t) {
promise.completeExceptionally(t) } }`), or give `supplyAsync` a real `Supplier`
(a Groovy lambda `() -> decorated.call()` compiles to invokedynamic, not a
proxy). Apply the same to `asCompletableFuture` and to the `Runnable` in
`WebPromises` (catch inside, stash, rethrow after `run()` returns). Add
`IOException` variants of the three new `CompletableFuturePromiseFactorySpec`
cases and one in `WebPromisesSpec`; they fail today.
### Medium
**N2. Standalone `WebPromises` propagates the request into `task {}` but not
into `then {}`.**
The M1 fix decorates the *closure* (`WebPromises.groovy:53-76`), so the
request is bound only for the duration of `work.call()` and reset in
`GrailsWebRequestTaskDecorator`'s `finally` before `Supplier.get()` returns.
`CompletableFuture.AsyncSupply.run()` then calls `postComplete()`, which is
where `then {}` dependents (`CompletableFuturePromise.groovy:63`, plain
`thenApply`) execute, so they run without a `GrailsWebRequest`. In 7.x
`FutureTaskChildPromise.groovy:57` applied the factory decorators to every
`then` callback, so `task { load() }.then { render it }` worked in
`ControllerUnitTest` specs; it now silently loses `render`/`params` in the
callback. The Boot path is fine because the `TaskDecorator` wraps the whole
`run()`, including `postComplete()`.
*Probe (`WebPromises.promiseFactory = null`, latch so `then` is attached
before the task completes):*
```
standalone: task sees request: true; then sees request: false
Boot-style executor + GrailsWebRequestTaskDecorator: then sees request: true
```
*Fix:* decorate the *executor*, not the closure, exactly as the plugin does:
when `PromiseFactoryBuilder` yields a `CompletableFuturePromiseFactory`, build
it over `Executors.newCachedThreadPool()` wrapped so each `Runnable` goes
through `GrailsWebRequestTaskDecorator`; keep the lookup-strategy path only for
third-party factories from `ServiceLoader`. That also removes the extra proxy
boundary from N1. Add a `then {}` case to `WebPromisesSpec` that attaches the
callback before the task completes (the current parameters test attaches
nothing).
**N3. `spring.mvc.async.request-timeout` now fires by default for
`WebPromises`, and a timeout renders as a logged 500, not a 503.**
Honouring the property is right, but the effect is now reachable in every
app: Tomcat's 30 s default applies to `WebPromises.task {}` where it used to be
infinite. When it fires, Spring's `TimeoutDeferredResultProcessingInterceptor`
sets `AsyncRequestTimeoutException` as the concurrent result,
`UrlMappingsInfoHandlerAdapter.groovy:94` rethrows it, and
`GrailsExceptionResolver.java:166` unconditionally sets status 500 and logs the
full stack trace at ERROR. Spring MVC's own resolvers map that exception to 503
without a stack trace. The docs (`asyncRequests.adoc:32`) say only that the
timeout "ends the response lifecycle". Please either map
`AsyncRequestTimeoutException` (it is an `ErrorResponse`) to its status code in
`GrailsExceptionResolver`, or document that the app should add
`"503"(controller: …, exception: AsyncRequestTimeoutException)` and that the 30
s Tomcat default now applies to eager web promises. A functional test that
times out a `WebPromises.task {}` and asserts th
e status would pin this.
### Low
**N4. Silently dropping a promise returned after the async request
completed.** `AsyncActionResultTransformer.groovy:51` returns `null` in that
case, so the action's result is discarded, the task keeps running against a
recycled request, and nothing is logged. `WebPromises.prepareAsyncRequest`
throws `IllegalStateException` for the same situation; the transformer should
be consistent (throw, or at least log at WARN) rather than swallow.
**N5. `GrailsAsyncWebRequest` naming and small nits.** The class is a static
helper, not a web request, and it sits next to the deprecated
`AsyncGrailsWebRequest`, which is going to confuse readers;
`AsyncRequestSupport` or folding the two static methods into `WebPromises`/the
transformer would be clearer. `GrailsAsyncWebRequest.groovy:41` uses `def`
under `@CompileStatic`; and the timeout property is re-read and re-parsed with
`DurationStyle` on every async request (`:44`). Reading the `WebMvcProperties`
bean once, or caching the parsed value, would avoid that.
**N6. Fallback for a mistyped `applicationTaskExecutor` is silent.**
`ControllersAsyncGrailsPlugin.groovy:76-79` catches
`NoSuchBeanDefinitionException` with the same bean name, which also matches
`BeanNotOfRequiredTypeException` when an app defines `applicationTaskExecutor`
as a plain `Executor`. That mirrors Boot's MVC behaviour, so it is acceptable,
but a DEBUG/INFO line saying which executor the promise factory ended up with
would make the fallback diagnosable.
**N7. Squash still pending (round 1 L5).** The branch is now 18 first-parent
commits including two upstream merges and the two URL-mapping commits that
cancel each other out.
---
### Post-merge addendum (`b65a64ed69`, merge of upstream `8.0.x`)
The merge itself is clean for this PR's own code: none of the files behind
N1–N7 changed, the `PromiseFactoryBuilder` conflict was resolved keeping
`build(Executor)`, the merged `VirtualThreadPromiseFactorySpec` was extended to
assert the builder ignores a supplied executor under the opt-in, and all three
module suites pass on the merged head. Two things do change.
**N8 (Medium). The `grails.async.promiseFactory=virtual-thread` opt-in now
bypasses everything this PR builds on.**
`PromiseFactoryBuilder.groovy:49-55` returns `new
VirtualThreadPromiseFactory()` before looking at the executor, and the merged
spec pins that. Under the opt-in, in a Boot app, `grailsPromiseFactory` is
therefore a `VirtualThreadPromiseFactory`, which:
- is built on `FutureTaskPromise`, which this PR marks `@Deprecated(since =
'8.0', forRemoval = true)` (`FutureTaskPromise.groovy:43`). Either the
deprecation is premature, or the new factory needs to move off it before it
ships in the same release.
- has no readable `executor` property (`private final ExecutorService
executorService`), so `EventBusFactoryBean` falls through to the synchronous
bus with the startup WARN. Round-1 H1 comes back for this mode, and
`events.adoc:32` is wrong for it.
- gets neither Boot's `TaskDecorator` (it owns its executor) nor a
`PromiseDecoratorLookupStrategy`, because `WebPromises.setPromiseFactory` no
longer attaches one (`WebPromises.groovy:60`). On `8.0.x` today the old
`setPromiseFactory` did attach `AsyncWebRequestPromiseDecoratorLookupStrategy`,
so `WebPromises.task { render … }` works with the virtual-thread factory there
and stops working with this PR. Same for `Promises.task {}` in a controller.
- keeps the old aggregate `onError` contract (returns an empty list instead
of failing), so the "consistent `ExecutionException` semantics" section of the
docs is only true for the default factory.
*Fix:* the simplest coherent option is to make `VirtualThreadPromiseFactory`
a thin `CompletableFuturePromiseFactory` over
`Executors.newVirtualThreadPerTaskExecutor()` (it then has the `executor`
property, the same `get()`/`onError` semantics, `CompletionStage`, and no
dependency on the deprecated classes), and have `ControllersAsyncGrailsPlugin`
wrap whatever executor the built factory owns with the `TaskDecorator` beans.
If that is out of scope for this PR, then at least restore the lookup strategy
in `WebPromises.setPromiseFactory` for factories that are not
`CompletableFuturePromiseFactory`, and say in `asyncPromises.adoc` that the
opt-in does not share Boot's executor, event bus or request propagation. Add a
plugin spec that sets the system property and asserts `grailsPromiseFactory`
still propagates the request.
**N9 (Low). Needs another merge or rebase before it can land.** `git
merge-tree` against the current `origin/8.0.x` conflicts in
`upgrading80x.adoc`: sections 62 and 63 were added upstream after this branch
merged, so "64. Promise Execution and Async Request Migration" will need
renumbering again. Nothing else conflicts.
---
### Round 1 status
| Round 1 | Status | Notes |
|---|---|---|
| H1 event bus went synchronous | Fixed | `EventBusFactoryBean` reads the
factory's `executor` property reflectively; `ExecutorEventBus` handles a plain
`Executor` via `execute()`, no shutdown ownership. Covered by
`TaskExecuterEventBusSpec`. |
| H2 `unwrap` stripped one cause level | Fixed | Only
`CompletionException`/`ExecutionException` with a non-null cause are unwrapped;
transformer passes the failure straight to `setErrorResult`. Covered with a
caused exception in both specs. See N1 for the remaining wrapper. |
| H3 `NoUniqueBeanDefinitionException` with a second executor | Fixed |
Resolved by name; spec registers `taskScheduler` + `otherExecutor`. |
| H4 single-threaded fallback | Fixed | `corePoolSize = 8`; two-promise
latch test. |
| M1 standalone `WebPromises` lost the request | Partly fixed | Task closure
OK; `then {}` not (N2). |
| M2 `get()` type differed between promise and child | Fixed | Both now
`ExecutionException(original)`, direct and timed; the sneaky-throw overrides
are gone. |
| M3 docs / upgrade note | Fixed | Upgrade section added (numbered 64 after
the merge, see N9), Servlet API marked deprecated. |
| L1 aggregate `onError` type | Fixed | Both callbacks get the original. |
| L2 timeout consistency | Fixed | Both paths honour
`spring.mvc.async.request-timeout` (see N3 for the consequence). |
| L3 task-after-completion guard | Fixed | `COMPLETED` request attribute set
from a completion handler, checked in both entry points. |
| L4 decorator applies to `@Async` | Documented | |
| L5 squash | Open | N7 |
### Verified as correct in this round
- `CompletableFuture` semantics for the fixed `get()` path: a `supplyAsync`
failure reaches `whenComplete` as `CompletionException`, `fromStage` completes
the promise with the unwrapped original, and both the direct promise and its
`thenApply` child report `ExecutionException(original)` from `get()` and
`get(timeout)`. Spec matches observed behaviour.
- `WebAsyncManager` removes itself on completion (Spring 6.2+), so the
`COMPLETED` attribute is needed; the completion handler runs in
`AsyncListener.onComplete`, before Tomcat recycles the request, and the
attribute is cleared with the request. Dispatch reuses the same request object
with no `onComplete` in between, so the guard does not trip mid-dispatch.
- `DurationStyle.detectAndParse` defaults to milliseconds for a bare number,
matching Boot's binding of `spring.mvc.async.request-timeout`; `spring-boot` is
on the plugin's compile classpath transitively via `grails-core`'s `api`
dependency.
- `ThreadPoolTaskExecutor` with `corePoolSize = 8` and the default unbounded
queue matches Boot's defaults as documented.
- `EventBusFactoryBean`'s reflective lookup:
`CompletableFuturePromiseFactory.executor` is a public final property;
GPars/RxJava factories have no such property and keep the previous synchronous
fallback.
- The `TaskDecorator`-on-executor design does propagate the request into
`then {}` dependents in Boot mode (probe P3 above), because `AsyncSupply.run()`
calls `postComplete()` inside the decorated `Runnable`.
- All three module suites and their code-style checks pass on a fresh,
cache-bypassed run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
