hk2166 opened a new pull request, #16786:
URL: https://github.com/apache/grails-core/pull/16786
## Description
Fixes the failure @chrisala reproduced in #15851, following the "true copy"
suggestion there. In 8.0, `GrailsWebRequestTaskDecorator` (#16230) gives a task
started during a request its own `GrailsWebRequest`, but that object still
points at the servlet request. Tomcat recycles the request once it completes. A
task that outlives the request, such as a fire-and-forget `task { }` that runs
`new Book(name: 'x')`, then reads a recycled request and gets
`IllegalStateException: The request object has been recycled and is no longer
associated with this facade`. Data binding is where this shows up: for a `Map`
source, `MimeTypeUtils.resolveMimeType` reads the current request's formats.
The binder catches the exception and records it as a binding error.
### Changes
- **The task sees the live request until the request completes, then a
copy:** `GrailsWebRequestTaskDecorator` now gives the task a request that is
the request it was started during until that request completes. After that it
is a copy of the request, taken as it completed (`CompletedHttpServletRequest`).
- **What the copy holds:** the attributes, parameters, headers, locales,
cookies, session, and what identifies the request (URI, method, addresses and
so on).
- **What the task sets:** attributes the task sets stay with the copy.
- **What throws:** reading the body, dispatching, starting async
processing, creating a session and `isUserInRole` need the live request, so
they throw an `IllegalStateException` that says the request has completed.
- **Tasks started from a task:** they share the request of the task that
started them.
- **Docs:** the async guide (`asyncPromises.adoc`) now describes what a task
sees before and after the request completes.
### Why the copy is taken when the request completes, not when the task
starts
A copy taken when the task starts would break tasks that the response waits
for, which must use the live request:
- `WebPromises.task { render ... }` is documented to render through the live
request and response while Spring MVC keeps the request open.
- `AsyncActionResultTransformer` reads `MODEL_AND_VIEW` from the live
request once the promise completes. A task that rendered into a copy would
leave nothing for it to read.
So the task works on the live request for as long as the request is
processing, and switches to a copy only once the container is about to recycle
it.
Detecting completion has two cases:
- **Normal requests:** `GrailsWebRequestFilter` calls `requestCompleted()`
at the end of the dispatch, and a request destruction callback on the captured
`GrailsWebRequest` takes the copy.
- **Requests processed asynchronously:** that call comes at the end of the
*first* dispatch, while the response is still being produced. So if the request
has started async processing, the callback registers an `AsyncListener`
instead, and the copy is taken in `onComplete`, before the container recycles
the request. The listener re-registers itself in `onStartAsync` if another
async cycle starts.
### Not covered
- **The response is not copied.** Nothing can be written to it after the
request completes, and the guide now says a task must not try.
- **The task's request is a stand-in:** while the request is processing, the
task's request delegates to it but is not the same object (a
`java.lang.reflect.Proxy` for `HttpServletRequest`). Code in a task that
unwraps the request or checks its class sees the proxy, not the container's
request.
- **Third-party promise factories:** the compatibility decoration in
`WebPromises` for third-party `PromiseFactory` implementations decorates the
work when it runs, not when it is submitted. If that happens after the request
completed, the copy may not be available, and the task behaves as it does today.
- **The original Spring Security trace** in #15851 (7.0.0-M5, no async) has
no reproduction, and this PR doesn't address it. This PR says "Fixes #15851"
because the reproducible case is fixed. If you'd rather keep the issue open for
the original trace, I'll change it to a plain reference.
### Tests
All in `GrailsWebRequestTaskDecoratorSpec`, through the decorator's public
`decorate` API. The tests use a request that fails once recycled, the way
Tomcat's facade does:
- A task that runs after the request completed reads its parameters,
headers, attributes, locale, session and URI. What the task sets stays with the
copy.
- A task that resolves the format of a `Map` binding source with
`MimeTypeUtils.resolveMimeType`, the path from the report, gets the request's
content type.
- A task still running when the request completes saw the live request until
then: what it set reached the request. Afterwards it sees the request as it
completed.
- A task of a request processed asynchronously renders through the live
request after the first dispatch ends. A task that runs after the async
processing completed sees what was rendered.
- A task started from a task sees the same request.
- After completion, `getInputStream()`, `getSession(true)`,
`getRequestDispatcher(String)`, `startAsync()` and `isUserInRole(String)` throw
an `IllegalStateException`.
- The existing test that checked the task's request was the very same object
now checks that the task sees, and writes to, the request while it is active.
On 8.0.x all six new tests fail with `The request object has been recycled
and is no longer associated with this facade`.
### Verification
- All tests in `grails-async` (the plugin) pass, as do the integration tests
of `grails-test-examples-async-events-pubsub-demo`, which render promises over
HTTP.
- `./gradlew cleanViolationReports aggregateViolations --continue` reports
no Checkstyle, CodeNarc, PMD or repository convention violations. SpotBugs is
disabled.
- I didn't run the repository-wide `clean` or `:grails-test-report:check`,
which would run every module's tests.
@codeconsole, @jdaugherty asked for your review since this builds on #16230.
Fixes #15851
Generative AI tooling (Claude Code) was used to research the cause, write
the change and its tests, and run the checks. I reviewed the change.
## Contributor Checklist
Please review the following checklist before submitting your pull request.
Pull requests that do not meet these requirements may be closed without review.
### Issue and Scope
- [x] This PR is linked to an existing issue that has been **acknowledged or
approved** by the project team. If no approved issue exists, please give
background on why this change is necessary. Tickets are preferred for release
change log history.
- [x] This PR addresses the **complete scope** of the linked issue. Partial
implementations or unfinished work should not be submitted for review. *(The
reproducible case. The original 7.0.0-M5 Spring Security trace has no
reproduction; see "Not covered".)*
- [x] This PR contains a **single, focused change**. Unrelated changes
should be submitted as separate pull requests.
- [x] This PR targets the **correct branch** for the type of change:
- **Patch release branches** (e.g., `7.0.x`): Bug fixes only. No new
features or API changes.
- **Minor release branches** (e.g., `7.1.x`): New features are welcome,
but breaking existing APIs must be avoided.
- **Major release branches** (e.g., `8.0.x`): Reserved for major
changes. Breaking API changes are permitted.
### Code Quality
- [x] I have **added or updated tests** that cover the changes introduced in
this PR. All code contributions are expected to include appropriate test
coverage.
- [ ] I have verified that all existing tests pass by running `./gradlew
build --rerun-tasks`. *(Not the full build; the affected module and the async
example app pass, see above.)*
- [x] My code follows the project's **code style** guidelines. I have run
`./gradlew codeStyle` and resolved any violations. See [Code
Style](../CONTRIBUTING.md#code-style) for details.
- [x] This PR does **not** include mass reformatting, style-only changes, or
large-scale refactoring unless it was **explicitly approved** in the linked
issue. Unsolicited reformatting will not be accepted.
- [x] If generative AI tooling was used in preparing this contribution, a
quality model was used to ensure contributions are **consistent with the
project's quality standards**.
### Licensing and Attribution
- [x] All contributed code is provided under the [Apache License
2.0](https://www.apache.org/licenses/LICENSE-2.0), and new source files include
the appropriate **Apache license header**.
- [ ] I have the necessary rights to submit this contribution and confirm it
is my own original work (see [Legal
Notice](../CONTRIBUTING.md#i-want-to-contribute)).
- [x] If generative AI tooling was used in preparing this contribution, I
have followed the [Apache Software Foundation's policy on generative
tooling](https://www.apache.org/legal/generative-tooling.html) and have
properly attributed its use.
### Documentation
- [x] If this PR introduces user-facing changes, I have included or updated
the relevant documentation.
- [x] If this PR adds a new feature, I have updated the **What's New**
section of the Grails Guide.
- [x] If this PR introduces breaking changes or changes that require user
action during an upgrade, I have updated the **Upgrade Notes** for the
corresponding version in the Grails Guide.
- [x] The PR description clearly explains **what** was changed and **why**.
---
> **First-time contributors:** Please read our [Contributing
Guide](../CONTRIBUTING.md) before submitting.
> Pull requests that appear to be auto-generated, incomplete, or unrelated
to an approved issue may be
> closed to help maintainers focus on reviewed and planned work. We
appreciate your understanding.
--
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]
