matrei commented on PR #16467:
URL: https://github.com/apache/grails-core/pull/16467#issuecomment-5956534523
@jdaugherty Thanks, that context helps.
**#1:** I don't think 7.x is protected here any more than 8.x is. The
wrapping you mention is the same on both lines. Spring 6.2's and 7.0's
`DispatcherServlet.doDispatch` wrap a non-`Exception` `Throwable` the same way:
I diffed 6.2.19 against 7.0.9, and only the last-modified handling moved. In
both versions, the wrapped exception reaches `afterCompletion` as `ex`, and the
attribute is never read. The crash happens when `ex` is null and the
`exception` attribute holds something that isn't an `Exception`. Everything
that can put such a value there exists on 7.x too: a model entry, or an error
handler that copies the container's `jakarta.servlet.error.exception` (see #2).
The `(Exception)` cast is identical on `7.0.x`
(`GrailsInterceptorHandlerInterceptorAdapter.groovy:122`).
So I think the Spring Session bug produced the `Error`, and the upgrade is
just where it surfaced. A 7.x app with the same failing filter, or with `model:
[exception: 'Something went wrong']`, crashes the same way. The fix is small,
has no API impact, and the tests are self-contained. Could you retarget it to
`7.0.x` and let it merge up?
**#2:** I believe you hit a real `Error` there. I think it reached the
attribute by a different route than the guide describes, though, and I'd like
the guide to describe the route you actually saw. Here is what I could and
couldn't find on `8.0.x`:
- `GrailsExceptionResolver.setStatus` is the only framework code that writes
`exception`, and it always writes a `GrailsWrappedRuntimeException`.
- Inside the dispatch, an `Error` is still wrapped in a `ServletException`
(Spring 7.0.9 `doDispatch`). When the resolver doesn't handle it,
`afterCompletion` receives it as `ex`, and the attribute is never read.
- An `Error` thrown outside the `DispatcherServlet` goes to the container. A
Spring Session commit is one example, because `SessionRepositoryFilter` commits
after the servlet returns. The container stores the bare `Error` in
`jakarta.servlet.error.exception`, not in `exception`, and then makes an ERROR
dispatch. `grailsInterceptorMappedInterceptor` is a `MappedInterceptor` and
nothing excludes ERROR dispatches, so the interceptors run again for the error
handler. That fits what you saw. But something in that error handler still has
to copy the `Error` into `exception`, for example a status-code controller that
renders `model: [exception:
request.getAttribute('jakarta.servlet.error.exception')]`.
Do you still have the stack trace, or can you tell what handled the 500 in
that app? If it is the ERROR dispatch path, I'd suggest:
- Guide: say that `throwable` may be any `Throwable`, including an `Error`
that reaches the container's error page from outside the action (for example
from a servlet filter). Also say that an `Error` thrown by an action arrives
wrapped, so you reach it through `getCause()`.
- Tests: one feature that follows that path, so the test shows the scenario
that actually happened.
Either way, the fix itself is correct. The suggestions in #2 are only about
making the docs match what users will see. The branch question in #1 is the
only thing I would like settled before it merges.
--
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]
