matrei commented on PR #16230:
URL: https://github.com/apache/grails-core/pull/16230#issuecomment-5697390108
## Review
**Head:** `8dd2b0a1dd` (`feature/modernize-grails-async-8.0.x`) · **Base:**
`8.0.x` @ `a5758d657f` · merge-base is clean (branch is a fast-forward
candidate; `origin/8.0.x` has since moved 91 commits, none touching async or
URL mappings).
**What I ran locally**
| Check | Result |
|---|---|
| `:grails-async-core:test` (fresh, `--no-build-cache`) | 54 tests, 0
failures, 1 skipped |
| `:grails-async:test` (fresh) | 18 tests, 0 failures |
| `:grails-web-url-mappings:test` (`--rerun`) | 259 tests, 0 failures |
| `:grails-async-core:codeStyle :grails-async:codeStyle
:grails-web-url-mappings:codeStyle` | pass |
| Throwaway probe specs (deleted afterwards) for the findings below | see
"Probe" notes per finding |
| Boot 4.1.1 bytecode (`OnBeanCondition`, `TaskExecutorConfigurations`,
`WebMvcAutoConfiguration`) | inspected with `javap` |
**Verdict: request changes.** The direction is right and the Spring MVC
`DeferredResult` integration is a clear improvement over the hand-rolled
`AsyncContext` handling, but the PR currently regresses four things that ship
enabled in every generated Grails app: the events bus silently becomes
synchronous, exceptions with a cause are misreported, the promise factory bean
fails to resolve when a second `AsyncTaskExecutor` exists (e.g.
`@EnableScheduling`), and the fallback executor is single-threaded.
---
### High
**H1. Every Grails app's `EventBus` silently becomes synchronous.**
`grails-events/core/src/main/groovy/org/grails/events/bus/spring/EventBusFactoryBean.groovy:73`
only reuses the promise factory when it `instanceof ExecutorService`.
`CachedThreadPoolPromiseFactory` was one (via `ExecutorPromiseFactory`);
`CompletableFuturePromiseFactory` is not. The factory bean therefore falls
through to `EventBusBuilder.createDefaultEventBus()`, which logs `No event bus
implementations found on classpath, using synchronous implementation.` and
builds `ExecutorEventBus(new SyncTaskExecutor())`. `grails-events` is added to
every forge-generated app (`grails-forge-core
.../feature/grails/GrailsBase.java:102`), so `sendEvent`/`@Subscriber`
listeners now run on the caller's thread with a spurious WARN at startup. The
updated `grails-doc/src/en/guide/async/events.adoc:32` claims the bus "executes
work on Spring Boot's `applicationTaskExecutor`", which is now false.
*Probe:* `EventBusFactoryBean` with a non-`ExecutorService`
`grailsPromiseFactory` singleton → `ExecutorEventBus.executor` is
`SyncTaskExecutor`.
*Fix:* `ExecutorEventBus` already accepts a plain `Executor`. Have
`EventBusFactoryBean` use `CompletableFuturePromiseFactory.executor` (or
introduce a small `ExecutorAware`-style interface on the factory) so the bus
shares the Boot executor as the docs say. Add a test in `grails-events` for it.
**H2. `unwrap` strips one cause level from *any* exception, so `onError` and
the error page see the wrong exception.**
`CompletableFuturePromise.groovy:144` (`failure.cause ?: failure`) is
applied in `onError` (line 75) to the raw exception the promise itself failed
with, not to a `CompletionException` wrapper. For `createPromise { throw new
IllegalStateException('outer', new IOException('inner')) }`, the `onError`
callback receives the `IOException`. `AsyncActionResultTransformer.groovy:72`
then applies its own `unwrap` (line 79) on top, so the exception handed to
`DeferredResult.setErrorResult` and ultimately to `GrailsExceptionResolver` /
UrlMappings exception mappings is the cause, not the thrown exception. Any
exception mapping keyed on the thrown type (`"500"(controller: 'errors',
exception: MyServiceException)`) will stop matching when that exception has a
cause.
*Probe:* both `Promise.onError` and `WebAsyncManager.concurrentResult`
observed `java.io.IOException: inner`.
*Fix:* only unwrap `CompletionException`/`ExecutionException` (and only when
`cause != null`), and drop the second unwrap in the transformer. Add tests with
a caused exception to `CompletableFuturePromiseFactorySpec` and
`AsyncActionResultTransformerSpec`.
**H3. `context.bean(AsyncTaskExecutor)` fails with
`NoUniqueBeanDefinitionException` whenever a second `AsyncTaskExecutor`
exists.**
`ControllersAsyncGrailsPlugin.groovy:72` resolves by type.
`ThreadPoolTaskScheduler` implements `AsyncTaskExecutor`, so any app with
`@EnableScheduling` (Boot auto-configures `taskScheduler`) or with its own
second executor bean fails on context startup with `expected single matching
bean but found 3: grailsPromiseExecutor,applicationTaskExecutor,taskScheduler`.
The fallback flag does not help here because there are two non-fallback
candidates. Boot itself avoids this:
`WebMvcAutoConfiguration$WebMvcAutoConfigurationAdapter.configureAsyncSupport`
looks up `applicationTaskExecutor` **by name** and does nothing otherwise.
*Probe:* registrar + `applicationTaskExecutor` + an initialized
`ThreadPoolTaskScheduler` singleton → `BeanCreationException` wrapping the
above.
*Fix:*
`context.beanFactory.containsBean(TaskExecutionAutoConfiguration.APPLICATION_TASK_EXECUTOR_BEAN_NAME)`
→ use it, else the `grailsPromiseExecutor` bean by name. Cover it with a test
that registers two executors.
**H4. The fallback executor is single-threaded.**
`ControllersAsyncGrailsPlugin.groovy:62` uses a default
`ThreadPoolTaskExecutor` (core 1, max `Integer.MAX_VALUE`, queue
`Integer.MAX_VALUE`). With an unbounded queue the pool never grows past the
core size, so every promise in a non-Boot context (the Micronaut path the PR
description mentions, plain Spring contexts, tests) is serialised on one
thread, and a promise that waits on another promise deadlocks. The previous
default was a cached thread pool.
*Probe:* promise A awaiting a latch that promise B releases → A times out;
executor reports `core=1 max=2147483647 queueCapacity=2147483647`.
*Fix:* mirror Boot's defaults (core 8 / unbounded queue), or
`SimpleAsyncTaskExecutor` (optionally virtual threads on Java 21), or simply
fall back to `new CompletableFuturePromiseFactory()` and wrap its executor with
the decorators. Add a two-promise concurrency test.
### Medium
**M1. `WebPromises` no longer propagates the request unless the plugin bean
has been instantiated.**
Previously `WebPromises.getPromiseFactory()` / `setPromiseFactory()` always
attached `AsyncWebRequestPromiseDecoratorLookupStrategy`, so `task { render ...
}` worked in controller unit tests and in any context where
`grailsPromiseFactory` had not been created yet. Now propagation lives only on
the Spring-registered executor; `WebPromises.groovy:50` builds a bare
`CompletableFuturePromiseFactory` with its own cached pool and no
`GrailsWebRequestTaskDecorator`. `grails-testing-support-web` does not load
`ControllersAsyncGrailsPlugin`, so `ControllerUnitTest` specs of async actions
lose `render`/`params`/`request` inside `task {}`. The rewritten
`WebPromisesSpec` masks this by wiring the decorator manually in `setup()`.
*Probe:* `WebPromises.promiseFactory = null`, bind a `GrailsWebRequest`,
`WebPromises.task { GrailsWebRequest.lookup() }.get()` → `null` (was a
`GrailsWebRequest`).
*Fix:* when `WebPromises` builds its default factory, wrap the executor with
`GrailsWebRequestTaskDecorator` (or keep a lookup strategy that applies it).
Restore a spec that does not pre-wire the decorator.
**M2. `get()` exception type differs between a promise and its `then {}`
child.**
`CompletableFuturePromiseFactory.groovy:81` passes `unwrapFailureOnGet =
false`, while `fromStage`'s default at `CompletableFuturePromise.groovy:127` is
`true`, and `onComplete`/`then` use the default. So `createPromise { throw X
}.get()` throws `ExecutionException(X)` (matching
`FutureTaskPromiseFactorySpec` and `PromiseListSpec`), but `createPromise {
throw X }.then { it }.get()` throws `X` directly. Lines 105/118 also
sneaky-throw a checked `Throwable` from a method declared `throws
ExecutionException`, which Java callers cannot catch by declaration. Pick one
behaviour (the legacy `ExecutionException` one keeps compatibility) and cover
it.
*Probe:* `direct=java.util.concurrent.ExecutionException
chained=java.lang.IllegalStateException`.
**M3. Docs still present the deprecated Servlet API as current, and the
upgrade guide is silent.**
`grails-doc/src/en/guide/async/asyncServletApi.adoc:25` and
`traits/traitsprovided.adoc:35` document `AsyncController`/`startAsync()` with
no deprecation notice, and `upgrading/upgrading80x.adoc` has nothing on: the
executor switch (pool size now governed by `spring.task.execution.*`, 8 core
threads instead of unbounded), the deprecations, the `ExecutorPromiseFactory`
removal from the default factory, or the events-bus change. Per the repo rule
that user-facing changes ship with docs, please add an 8.0 upgrade note and
mark the Servlet section deprecated.
### Low
**L1. `onError(List, Closure)` hands the callback a freshly built
`ExecutionException(cause)`** (`CompletableFuturePromiseFactory.groovy:128`)
while single-promise `onError` hands over the unwrapped cause. Callers of
`Promises.onError(list) { e -> }` and `promise.onError { e -> }` get different
types for the same failure. Align them.
**L2. Timeout inconsistency between the two entry points.**
`WebPromises.prepareAsyncRequest()` (`WebPromises.groovy:192`) sets an infinite
timeout, whereas an action returning a `Promises.task {}` / GORM `Domain.async`
promise goes through the transformer with the container default (Tomcat: 30 s).
This mirrors the old split, so not a regression, but now that the flow is
Spring-managed it would be natural to honour `spring.mvc.async.request-timeout`
via `DeferredResult(timeout)` in both paths.
**L3. The "task after async completion" guard is gone.** The old decorator
threw `Cannot start a task once asynchronous request processing has completed`;
now such a task captures and later touches a recycled servlet request. Consider
checking `asyncManager.asyncWebRequest?.isAsyncComplete()` in
`prepareAsyncRequest()`.
**L4. `grailsWebRequestTaskDecorator` is a global `TaskDecorator` bean**, so
Boot also applies it to `@Async` methods and (Boot ≥ 3.5) the task scheduler.
Harmless when no request is bound, but it is a behaviour change for `@Async`
methods called from controllers (they now see a `GrailsWebRequest` copy). Worth
a sentence in the docs.
**L5. Commit history.** `479f5e0e2e Restore URL mapping state on async
dispatch` and `c982feb634 Reuse matched URL mappings on async dispatch` cancel
each other out (`UrlMappingsHandlerMapping` ends up unchanged). Please squash
before merge per the contribution guide.
---
### Verified as correct
- **Fallback bean does not suppress Boot's executor.** `OnBeanCondition` in
Boot 4.1.1 filters fallback definitions (`getNonFallbackBeans`/`isNotFallback`
→ `BeanDefinition.isFallback()`), and
`TaskExecutorConfigurations$OnExecutorCondition$ExecutorBeanCondition` is
`@ConditionalOnMissingBean(Executor.class)`, so `grailsPromiseExecutor` does
not stop `applicationTaskExecutor` from being created.
- **Boot composes multiple `TaskDecorator` beans**:
`ThreadPoolTaskExecutorBuilderConfiguration` takes
`ObjectProvider<TaskDecorator>`, uses `orderedStream()` and
`getTaskDecorator(...)`, so `GrailsWebRequestTaskDecorator` is applied
alongside user decorators on `applicationTaskExecutor`.
- **`AsyncActionResultTransformer` + `UrlMappingsInfoHandlerAdapter`** now
follow the same protocol as Spring's `RequestMappingHandlerAdapter`
(`hasConcurrentResult()` → read → `clearConcurrentResult()` → rethrow
`Exception` / return `ModelAndView`), and a `null` concurrent result correctly
falls through to "no view" for actions that `render` inside the task (the
`AsyncFunctionalSpec` path).
- **Joining an eagerly started async cycle**:
`StandardServletAsyncWebRequest.startAsync()` is a no-op when already started,
so `WebPromises.task {}` followed by returning the promise works (covered by
the new transformer spec).
- **Executor affinity**: `defaultExecutor()` + `newIncompleteFuture()` keep
`thenApplyAsync` etc. on the configured executor; the decorator therefore also
covers derived stages.
- **Explicit decorators are honoured** by `WebPromises.createPromise(c,
decorators)` — the old code silently replaced them with the lookup result, so
this is a bug fix.
- **`@PreDestroy close()`** only shuts down an executor the factory owns, so
the Boot-managed executor is left to Boot's lifecycle.
- All three affected module suites and their code-style checks pass on a
fresh 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]
