jdaugherty commented on PR #16786:
URL: https://github.com/apache/grails-core/pull/16786#issuecomment-6505548894
> Requesting changes. My objection is to the approach more than to any one
line.
>
> The bug in #15851 is a task that outlives its request and then reads the
recycled facade. The trigger is data binding asking `MimeTypeUtils` for the
request's formats, which, as chrisala notes, the binder doesn't actually need.
This PR fixes it by having the task read a reimplementation of
`HttpServletRequest` once the request completes. That's about 2,500 lines of
new main code (`CompletedHttpServletRequest`, `TaskRequest`, `DispatchView`,
and a filter on every request). It covers body buffering, reader mark/reset and
`\r` handling, RFC 850/asctime date parsing, forward-attribute shadowing,
multipart stand-ins, and a read/write lock around every call a task makes on
the request. Each of the seven review rounds found a new place where the copy
differs from the live request, and each fix made the emulation bigger. I don't
see that converging. Every container, wrapper and filter is another way for the
copy to drift from the request it replaces, and this code would have to keep up
with all of them on 8.0.x.
>
> Spring has already defined what a request looks like after it completes.
Once `ServletRequestAttributes.requestCompleted()` has run, request-scope
access throws `IllegalStateException("Cannot ask for request attribute -
request is not active anymore!")`. hk2166's first proposal on the issue matches
that: when the request completes, detach the task's web request.
`GrailsWebRequest.lookup()` then returns null in the task, and code that can
run without a request (binding, `MimeTypeUtils`, the binder's locale) falls
back the way it already does on a plain background thread. That's also how
`Promises.task` behaved in 7.x. Detaching keeps what this PR preserves while
the request is still being processed: a `WebPromises.task` that renders while
Spring MVC holds the request open, and `AsyncActionResultTransformer` reading
`MODEL_AND_VIEW`. Both keep working because the task still has the request
object itself until completion. Detaching also covers the response, which this
PR leaves poi
nting at the recycled facade (inline), and it needs no body buffering, no
proxy, and no emulation that has to match the container.
>
> @jdaugherty, you suggested a true copy on the issue. After how the rounds
went, I'd rather go back to detaching: a task that outlives its request should
behave like code outside a request, not like a second request. Is there a case
you had in mind that only a copy covers?
>
> If the copy stays, the inline comments are what I'd still need addressed.
I confirmed two of them with a throwaway spec on
[3847d6f](https://github.com/apache/grails-core/commit/3847d6f006e8e169ada96ee556e862da03aa4fa6):
>
> * An unread 1 MB `application/octet-stream` body, with a task started
that never touches the request: the thread completing the request reads all
1,048,576 bytes inside the filter chain. With a 16 MB body it reads 2,097,153
bytes and keeps none of them.
>
> * Inside a task, while the request is still live,
`org.springframework.web.util.WebUtils.getNativeRequest(request,
MockHttpServletRequest)` returns `null`, and the task's request is no longer
the request object. On 8.0.x it is.
@codeconsole Yes, the reason I was suggesting a copy instead is because
those requests are managed by the dispatcher, etc and are meant to be closed by
the encompassing web request. If we aren't copying, we've now got state being
managed externally from the web thread process and there's a possibility we
won't have centralized logic. Historically, these type of changes are sources
of CVEs so copy seems like a more reasonable solution.
--
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]
