hk2166 commented on issue #15851:
URL: https://github.com/apache/grails-core/issues/15851#issuecomment-6449609412
I'd like to work on this. I traced @chrisala's reproduction on 8.0.x, and
its cause is different from the Spring Security trace in the original report.
**Cause:** since #16230 (in 8.0.0), `GrailsWebRequestTaskDecorator` gives
each `task { }` its own copy of the `GrailsWebRequest`. That copy still points
at the servlet request after the request has completed. Binding a Map reads the
current request even though the source is not the request:
`new Domain(name: 'x')` → `DataBindingUtils.bindObjectToDomainInstance` →
`createDataBindingSource` → `MimeTypeUtils.resolveMimeType` →
`DefaultMimeTypeResolver.resolveRequestMimeType()` →
`HttpServletRequestExtension.getMimeTypes(request)` →
`request.getAttribute(...)`
Once Tomcat has recycled the request, that call throws
`IllegalStateException`. `bindObjectToDomainInstance` catches it and records it
as a binding error, which is what chrisala saw. On 7.x, `Promises.task` didn't
carry the request into the task (only `WebPromises` did, and it kept the
request open with servlet async), so binding found no request and used
`MimeType.ALL`.
**Proposal:** detach the task's copy when the originating request completes.
The decorator would register a request destruction callback on the captured
request that marks the copy as completed, and `GrailsWebRequest.lookup()` would
stop returning a completed copy. The task then behaves like code running
outside a request: binding falls back to `MimeType.ALL` and the default locale,
as it already does on a plain background thread. It would also stop `lookup()`
from handing a task a request object that Tomcat has reused for a later request
when facade discarding is turned off.
A narrower option is to guard only the places where binding reads the
request (`DefaultMimeTypeResolver.resolveRequestMimeType` and
`GrailsWebDataBinder.getLocale`), but other framework code that looks up the
request inside a task would still fail.
Tests would cover a task that binds a Map to a domain instance after the
request completes (no binding errors), a task that still sees the request while
it is active, and chained `then { }` callbacks.
This would target 8.0.x, since the decorator only exists there. #16516 adds
an unrelated method to `GrailsWebRequest`, and #16596 converts
`grails-web-common` to Groovy on a side branch.
I haven't found a cause for the Spring Security trace in the original report
(7.0.0-M5, no async), so I'd leave that part open unless someone can reproduce
it.
@codeconsole @jdaugherty does detaching the task's copy fit the design of
#16230, or would you prefer the narrower guard?
--
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]
