jdaugherty commented on PR #16223: URL: https://github.com/apache/grails-core/pull/16223#issuecomment-5416064753
> Putting `RequestContextHolder.resetRequestAttributes()` in a finally block is correct defensive cleanup. Even if an earlier lifecycle bug caused the timing, cleanup of a thread-local must not depend on successful access to a potentially expired servlet request. Agreed, and you've convinced me — that half is now kept. 49b0bc97a8 puts the reset back in a `finally`. You're right that it's an unconditional postcondition and must not depend on the servlet request still being reachable. > Catching the failure from removeAttribute is also reasonable: once the request is recycled, there is no meaningful attribute left for Grails to remove. This is the half I still disagree with, for two reasons. **It generalises from one call site.** "Once the request is recycled" describes exactly one of this method's four callers: | Caller | State of the request when it is called | |---|---| | `GrailsAsyncContext:71` | completed — the case the change was written for | | `UrlMappingUtils:398` | **alive** — clears, then immediately `dispatcher.include(request, …)` | | `GrailsWebRequestFilter:92` | alive — inside its own `finally`, before the container recycles | | spring-security `AnnotationFilterInvocationDefinition:141` | alive — mid filter chain | `UrlMappingUtils` is the clearest: it clears the web request and hands *that same request* to `dispatcher.include` on the very next line. An `IllegalStateException` there isn't a recycled request, it's a bug — and the catch swallows it immediately before the request gets used again. `clearGrailsWebRequest` is public API, so this extends to callers outside the repo as well. **It puts state inference in a method with no authority over that state.** `clearGrailsWebRequest` is a static utility told to clear two locations. With the catch it reads one exception type as proof of a particular container lifecycle state, then decides that state is unremarkable. `IllegalStateException` out of `getRequest()`/`removeAttribute` isn't a unique signal for "recycled" — anything else throwing it in that path gets reclassified as expected. The `finally` makes no inference at all, which is exactly why it's the right half and the catch isn't. So the method is now `try`/`finally` with no `catch`: the thread is unbound either way, and a request that can't be reached is reported to the caller that owns the lifecycle instead of being absorbed here. Two tests in `WebUtilsTests` cover both halves — the thread is unbound when the request can't be reached, and the failure still propagates. Removing the `finally` fails the first. On the promise decorator, I'd still like it reverted. Worth noting your own analysis in #16219 rules it out as the cause of the `AsyncPromiseSpec` timeouts — two of the four occurrences predate it by three and a half weeks, one of them on `8.0.x`. So the catch there isn't fixing that flake; it's making a promise run undecorated with nothing at any visible log level to say so. If there's a case it does fix, I'd rather see it fixed at the lifecycle that leaves a request in that state. -- 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]
