codeconsole commented on code in PR #16786: URL: https://github.com/apache/grails-core/pull/16786#discussion_r4416133600
########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/CompletedHttpServletRequest.groovy: ########## @@ -0,0 +1,1274 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.web.async + +import java.nio.charset.Charset +import java.nio.charset.StandardCharsets +import java.security.Principal +import java.text.ParseException +import java.text.SimpleDateFormat +import java.util.concurrent.ConcurrentHashMap + +import groovy.transform.CompileStatic +import groovy.transform.PackageScope + +import jakarta.servlet.AsyncContext +import jakarta.servlet.DispatcherType +import jakarta.servlet.ReadListener +import jakarta.servlet.RequestDispatcher +import jakarta.servlet.ServletConnection +import jakarta.servlet.ServletContext +import jakarta.servlet.ServletInputStream +import jakarta.servlet.ServletRequest +import jakarta.servlet.ServletRequestWrapper +import jakarta.servlet.ServletResponse +import jakarta.servlet.http.Cookie +import jakarta.servlet.http.HttpServletMapping +import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletResponse +import jakarta.servlet.http.HttpSession +import jakarta.servlet.http.HttpUpgradeHandler +import jakarta.servlet.http.MappingMatch +import jakarta.servlet.http.Part + +import org.springframework.http.HttpHeaders +import org.springframework.http.HttpMethod +import org.springframework.util.LinkedCaseInsensitiveMap +import org.springframework.util.LinkedMultiValueMap +import org.springframework.util.MultiValueMap +import org.springframework.web.context.request.RequestAttributes +import org.springframework.web.multipart.MultipartFile +import org.springframework.web.multipart.MultipartHttpServletRequest + +import org.grails.web.util.WebUtils + +/** + * A copy of a request, taken as the request completed, which a task that outlives the request reads in its place. + * + * <p>The container recycles a request once it completes, so a task that read the request itself from then on + * would fail. As the request completes, a copy is taken of each request its tasks were given, such as the request + * and the request of a forward, and each task reads the copy of its request through a {@link #newView view} of its + * own, so each has the part of the body nothing had read, served through one stream or reader whose position + * persists, as the live request serves it. A copy holds the attributes, parameters, headers, locales and cookies of + * its request, the session and what identifies the request. The request, its response and what holds either, such + * as the web request of the request thread, are not carried over, so the copy holds nothing the container recycles. + * What only the live request can do, such as dispatching, processing asynchronously, creating a session or checking + * a role, is not available.</p> + * + * <p>A copy keeps how its request was dispatched when its task was given it, its path, forward and include + * attributes and, for a forward or an include, its parameters, though the container restores a request it wrapped + * for a forward as the forward returns. A copy taken as asynchronous processing completes, outside the filters, + * keeps who made the request as its last dispatch told, since a security filter clears that as the dispatch + * ends.</p> + * + * <p>The copies share the attributes of the request the container created and the body, as the requests share + * them, so attributes a task sets stay with the copies, shared by every task. What a request keeps itself, such as + * the forward attributes of the request a forward dispatched, stays with its own copy, as do the attributes it + * hides, and setting or removing one of them changes that copy only, as on the request.</p> + * + * <p>The parameters are those of the request. A multipart body the container could not parse leaves the copy + * without parameters, as params has none, and any other failure to read them is thrown to a task that reads them, + * as the live request throws it. Only a multipart request resolving lazily that can no longer parse the body as the + * request completes gives way to the request it wraps.</p> + * + * <p>The uploads of a multipart request are deleted as the request ends, so the copy answers as a multipart + * request without uploads when the request was a multipart request, and a copy of a multipart request an attribute + * held, such as the one the dispatcher resolved, takes its place, with its parameters, such as the text fields only + * it has. Reading params then finds the parameters it found on the live request and no uploads.</p> + * + * <p>The body is kept up to {@value #BODY_LIMIT} bytes. Reading a longer one from the copy throws, so the copy is + * not what holds an upload that was never read in memory. The body of a multipart request is not kept: its uploads + * go with the request, and its text fields are in the parameters.</p> + * + * @since 8.0.1 + */ +@CompileStatic +class CompletedHttpServletRequest implements MultipartHttpServletRequest { + + /** + * The longest body, in bytes, a copy keeps: what remains of a longer one is not read. + */ + static final int BODY_LIMIT = 2 * 1024 * 1024 + + private static final byte[] NO_BODY = new byte[0] + private static final int READ_CHUNK = 8192 + // An attribute the request of a copy hides, though the request the container created has it + private static final Object HIDDEN = new Object() + private static final List<String> DATE_HEADER_FORMS = List.of( + 'EEE, dd MMM yyyy HH:mm:ss zzz', + 'EEEE, dd-MMM-yy HH:mm:ss zzz', + 'EEE MMM dd HH:mm:ss yyyy') + + private final Map<String, Object> attributes + private final Map<String, Object> ownAttributes + // Null when the body was longer than the copy keeps + private final byte[] body + // What reading the rest of the body failed with, which a read past what was copied throws, as on the request + private final Exception bodyFailure + private final Map<String, String[]> parameters + private final RuntimeException parameterFailure + private final Map<String, List<String>> headers + private final List<Locale> locales + private final Cookie[] cookies + private final HttpSession session + private final ServletContext servletContext + private final Principal userPrincipal + private final DispatcherType dispatcherType + private final HttpServletMapping httpServletMapping + private final ServletConnection servletConnection + private final String authType + private final String contentType + private final long contentLength + private final String method + private final String requestURI + private final String requestURL + private final String queryString + private final String contextPath + private final String servletPath + private final String pathInfo + private final String pathTranslated + private final String protocol + private final String scheme + private final String serverName + private final int serverPort + private final boolean secure + private final String remoteAddr + private final String remoteHost + private final int remotePort + private final String localName + private final String localAddr + private final int localPort + private final String remoteUser + private final String requestedSessionId + private final boolean requestedSessionIdValid + private final boolean requestedSessionIdFromCookie + private final boolean requestedSessionIdFromURL + private final String requestId + private final String protocolRequestId + private volatile String characterEncoding + private ServletInputStream inputStream + private BufferedReader reader + + private CompletedHttpServletRequest(HttpServletRequest request, Map<String, Object> attributes, + Map<String, Object> ownAttributes, CopiedParameters parameters, CopiedBody body, DispatchView view, + Identity identity) { + this.attributes = attributes + this.ownAttributes = ownAttributes + this.parameters = parameters.values + this.parameterFailure = parameters.failure + this.body = body.bytes + this.bodyFailure = body.failure + Map<String, List<String>> headers = new LinkedCaseInsensitiveMap<>(Locale.ENGLISH) + for (String name : Collections.list(request.headerNames)) { + headers.put(name, Collections.unmodifiableList(Collections.list(request.getHeaders(name)))) + } + this.headers = Collections.unmodifiableMap(headers) + this.locales = Collections.unmodifiableList(Collections.list(request.locales)) + this.cookies = copyCookies(request.cookies) + this.session = request.getSession(false) + this.servletContext = request.servletContext + // As the request was dispatched when the task was given it, though the container restores a request it + // wrapped for a forward as the forward returns + DispatchView dispatch = view ?: new DispatchView(request) + // Who made the request, as the filters processing it told, though a security filter clears it + Identity user = identity ?: new Identity(request) + this.userPrincipal = user.userPrincipal + this.dispatcherType = dispatch.dispatcherType + this.httpServletMapping = dispatch.httpServletMapping + this.servletConnection = copyConnection(request.servletConnection) + this.authType = user.authType + this.characterEncoding = request.characterEncoding + this.contentType = request.contentType + this.contentLength = request.contentLengthLong + this.method = request.method + this.requestURI = dispatch.requestURI + this.requestURL = dispatch.requestURL + this.queryString = dispatch.queryString + this.contextPath = dispatch.contextPath + this.servletPath = dispatch.servletPath + this.pathInfo = dispatch.pathInfo + this.pathTranslated = dispatch.pathTranslated + this.protocol = request.protocol + this.scheme = request.scheme + this.serverName = request.serverName + this.serverPort = request.serverPort + this.secure = request.secure + this.remoteAddr = request.remoteAddr + this.remoteHost = request.remoteHost + this.remotePort = request.remotePort + this.localName = request.localName + this.localAddr = request.localAddr + this.localPort = request.localPort + this.remoteUser = user.remoteUser + this.requestedSessionId = request.requestedSessionId + this.requestedSessionIdValid = request.requestedSessionIdValid + this.requestedSessionIdFromCookie = request.requestedSessionIdFromCookie + this.requestedSessionIdFromURL = request.requestedSessionIdFromURL + this.requestId = request.requestId + this.protocolRequestId = request.protocolRequestId + } + + // Field by field: the getters serve tasks, so some, such as getSession and getLocales, do not return the field + private CompletedHttpServletRequest(CompletedHttpServletRequest copy) { + this.attributes = copy.@attributes + this.ownAttributes = copy.@ownAttributes + this.body = copy.@body + this.bodyFailure = copy.@bodyFailure + this.parameters = copy.@parameters + this.parameterFailure = copy.@parameterFailure + this.headers = copy.@headers + this.locales = copy.@locales + this.cookies = copy.@cookies + this.session = copy.@session + this.servletContext = copy.@servletContext + this.userPrincipal = copy.@userPrincipal + this.dispatcherType = copy.@dispatcherType + this.httpServletMapping = copy.@httpServletMapping + this.servletConnection = copy.@servletConnection + this.authType = copy.@authType + this.characterEncoding = copy.@characterEncoding + this.contentType = copy.@contentType + this.contentLength = copy.@contentLength + this.method = copy.@method + this.requestURI = copy.@requestURI + this.requestURL = copy.@requestURL + this.queryString = copy.@queryString + this.contextPath = copy.@contextPath + this.servletPath = copy.@servletPath + this.pathInfo = copy.@pathInfo + this.pathTranslated = copy.@pathTranslated + this.protocol = copy.@protocol + this.scheme = copy.@scheme + this.serverName = copy.@serverName + this.serverPort = copy.@serverPort + this.secure = copy.@secure + this.remoteAddr = copy.@remoteAddr + this.remoteHost = copy.@remoteHost + this.remotePort = copy.@remotePort + this.localName = copy.@localName + this.localAddr = copy.@localAddr + this.localPort = copy.@localPort + this.remoteUser = copy.@remoteUser + this.requestedSessionId = copy.@requestedSessionId + this.requestedSessionIdValid = copy.@requestedSessionIdValid + this.requestedSessionIdFromCookie = copy.@requestedSessionIdFromCookie + this.requestedSessionIdFromURL = copy.@requestedSessionIdFromURL + this.requestId = copy.@requestId + this.protocolRequestId = copy.@protocolRequestId + } + + /** + * Copies the requests the tasks of a request that is completing were given: the request, and any other request + * a later task was given, such as the request of a forward. It has to be called before the container recycles + * the request. Each copy holds what its request shows, and the copies share the attributes of the request the + * container created and the body, as the requests share them. + * + * @param requests the requests, each the request the container created or a wrapper of it, the first being the + * one whose body the copies share + * @param views how each request was dispatched when a task was given it + * @param identities who made the request, as the filters processing it told, for each request, or {@code null} + * entries when the request itself still tells + * @param takenBody the live stream or reader a task took to read the body, which the rest of the body is read + * from, or {@code null} + * @return the copies, in the order of the requests, with {@code null} for a request after the first that could + * not be copied + */ + static List<CompletedHttpServletRequest> copyOf(List<HttpServletRequest> requests, List<DispatchView> views, + List<Identity> identities, TakenBody takenBody) { + Map<String, Object> underlying = attributesOf(TaskRequest.underlying(requests.get(0))) + // The requests to copy: those given, then each multipart request an attribute holds, such as the one the + // dispatcher resolved, which wraps the request, so its copy, which has no uploads either, takes its place + List<HttpServletRequest> copied = new ArrayList<>(requests) + Map<Object, Integer> multipartRequests = new IdentityHashMap<>() + for (int i = 0; i < requests.size(); i++) { + if (requests.get(i) instanceof MultipartHttpServletRequest) { + multipartRequests.putIfAbsent(requests.get(i), i) + } + } + addMultipartRequests(underlying, copied, multipartRequests) + List<Map<String, Object>> shownOtherwise = new ArrayList<>() + for (int i = 0; i < copied.size(); i++) { + Map<String, Object> shown = differences(attributesOf(copied.get(i)), underlying, viewOf(views, i)) + shownOtherwise.add(shown) + addMultipartRequests(shown, copied, multipartRequests) + } + // Every parameter map before the body: parsing the parameters consumes a form body, on the live request too + List<CopiedParameters> parameters = new ArrayList<>() + for (int i = 0; i < copied.size(); i++) { + parameters.add(viewOf(views, i)?.parameters ?: copyParameters(copied.get(i))) + } + CopiedBody body + try { + body = new CopiedBody(copyBody(requests.get(0), takenBody), null) Review Comment: This reads whatever the request left unread, for every request that started a task, whether or not any task ever reads the body. It runs on the thread completing the request, inside the filter chain, while holding the `RequestState` monitor and the copy's write lock. With a throwaway spec on this head: a `PUT` of 1 MB of `application/octet-stream` that the action never reads, plus an audit-style task that never touches the request. The completing thread reads all 1,048,576 bytes and holds them until the tasks are done. With 16 MB it reads 2,097,153 bytes and then discards them all, since the body is over the limit. Unless the action flushed, Tomcat sends the response only after the filter chain returns. So a client whose upload the action rejected (401, 413) now waits for up to 2 MB to arrive before it gets the response, and Tomcat then swallows up to another `maxSwallowSize` on top of that. Meanwhile, every task that calls the request is blocked on the write lock. If the copy stays, at the very least it should copy the body only when a task has already taken the stream or reader (`takenBody != null`). Otherwise, reading the body from the copy should throw, like the other things the copy doesn't keep. ########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/TaskRequest.groovy: ########## @@ -0,0 +1,995 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.web.async + +import java.lang.reflect.InvocationHandler +import java.lang.reflect.InvocationTargetException +import java.lang.reflect.Method +import java.lang.reflect.Modifier +import java.lang.reflect.Proxy +import java.util.concurrent.atomic.AtomicReference +import java.util.concurrent.locks.ReentrantReadWriteLock + +import groovy.transform.CompileStatic +import groovy.transform.PackageScope + +import jakarta.servlet.AsyncEvent +import jakarta.servlet.AsyncListener +import jakarta.servlet.ReadListener +import jakarta.servlet.ServletInputStream +import jakarta.servlet.ServletRequest +import jakarta.servlet.ServletRequestWrapper +import jakarta.servlet.http.HttpServletRequest + +import org.springframework.web.context.request.RequestAttributes + +import org.grails.web.servlet.mvc.GrailsWebRequest + +/** + * The request a task started during a web request sees. + * + * <p>Until the request completes, it is the request the task was given, with every public view that request and + * the requests it wraps offer, such as a multipart wrapper's, so a task the response waits for, such as one started + * with {@code WebPromises}, renders through it. Tasks of one request can be given different requests, such as the + * request and the request of a forward, and each task keeps its own. The container recycles the request once it + * completes, including any asynchronous processing, so from then on each task sees a + * {@link CompletedHttpServletRequest copy} of its request taken as the request completed, and the recycled request + * is released. The copies are taken once for the request, however many tasks it has, and each task reads its copy + * through a view of its own, so each has the body to read. A multipart request stays one, without the uploads, + * which go with the request.</p> + * + * <p>A task keeps how its request was dispatched when it started: its path, the forward and include attributes and, + * for a forward or an include, its parameters, though a container that gave the forward the very wrapper the + * request had, with its own wrapper inserted underneath, restores that wrapper as the forward returns. A task that + * is reading the body when the request completes reads on from its view of the copy, where it stopped, and a mark + * it set still holds. A task's calls to the live request and the copy do not interleave, so what a task sets while + * the copy is taken is in it.</p> + * + * <p>A request completes when the dispatch the container started for it ends, as {@link TaskRequestFilter} tells, + * after any forward or include within it, though each of those completes the web request it bound or reused as it + * returns. Without the filter, such as in a unit test, the request completes with the web request the first of its + * tasks was started during.</p> + * + * @since 8.0.1 + */ +@CompileStatic +class TaskRequest implements InvocationHandler { + + private static final String STATE = TaskRequest.name + '.STATE' + private static final String PROCESSING = TaskRequest.name + '.PROCESSING' + private static final String COMPLETION_CALLBACK = TaskRequest.name + '.COMPLETION' + + private final GivenRequest given + private CompletedHttpServletRequest view + private ServletInputStream body + private BufferedReader bodyReader + + private TaskRequest(GivenRequest given) { + this.given = given + } + + /** + * The request a task started during a web request sees. It has to be called while the web request is the + * current one, when the task is submitted. + * + * @param webRequest the web request the task is started during + * @return the request for the task + */ + static HttpServletRequest of(GrailsWebRequest webRequest) { + HttpServletRequest current = webRequest.currentRequest + if (Proxy.isProxyClass(current.getClass())) { + InvocationHandler handler = Proxy.getInvocationHandler(current) + if (handler instanceof TaskRequest) { + // A task started from a task reads the request that task was given through a view of its own + return (HttpServletRequest) Proxy.newProxyInstance(current.getClass().classLoader, + current.getClass().interfaces, new TaskRequest(((TaskRequest) handler).given)) + } + } + ClassLoader loader = current.getClass().classLoader ?: TaskRequest.classLoader + return (HttpServletRequest) Proxy.newProxyInstance(loader, interfacesOf(current, loader), + new TaskRequest(givenOf(webRequest, current, DispatchView.of(current)))) Review Comment: On 8.0.0 the task gets the request object itself; the existing test asserted exactly that. Here the task gets a `java.lang.reflect.Proxy` from the moment it starts, while the request is still live. So on a patch release, any code in a task that unwraps the request or checks its class behaves differently. In a throwaway spec on this head, `org.springframework.web.util.WebUtils.getNativeRequest(request, MockHttpServletRequest)` inside a task returns `null`, and `request.is(original)` is false. The same applies to `getNativeRequest(request, ContentCachingRequestWrapper)`, which request-logging setups rely on, to `instanceof HttpServletRequestWrapper`, and to any helper that walks `ServletRequestWrapper.getRequest()`. The proxy is there only so the live request and the copy can be swapped under the task. So are the reflective dispatch, the class-loader visibility check and dropping `GroovyObject`. If the task kept the real request until completion and was detached afterwards, none of it would be needed. ########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/GrailsWebRequestTaskDecorator.groovy: ########## @@ -45,12 +53,13 @@ class GrailsWebRequestTaskDecorator implements TaskDecorator { return task } + HttpServletRequest request = TaskRequest.of(captured) + // Taken now, so the task does not hold the web request, whose request the container recycles + HttpServletResponse response = captured.currentResponse Review Comment: The task's web request still holds the container's response, which Tomcat recycles together with the request. So the failure this PR fixes is still there for anything in a task that touches the response after completion, and that includes reads such as `response.contentType`, `response.committed` or `response.characterEncoding`. Tomcat's `ResponseFacade` throws the same "has been recycled and is no longer associated with this facade" `IllegalStateException` for those. The guide tells tasks not to write to the response, but nothing stops a read, and the comment above says the point is for the task not to hold what the container recycles. Detaching the task's web request when the request completes would cover the request and the response the same way. -- 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]
