hk2166 commented on code in PR #16786: URL: https://github.com/apache/grails-core/pull/16786#discussion_r4411488901
########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/TaskRequest.groovy: ########## @@ -0,0 +1,319 @@ +/* + * 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 groovy.transform.CompileStatic +import groovy.transform.PackageScope + +import jakarta.servlet.AsyncEvent +import jakarta.servlet.AsyncListener +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> + * + * @since 8.0.1 + */ +@CompileStatic +class TaskRequest implements InvocationHandler { + + private static final String STATE = TaskRequest.name + '.STATE' + private static final String COMPLETION_CALLBACK = TaskRequest.name + '.COMPLETION' + + private final GivenRequest given + private CompletedHttpServletRequest view + + 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))) + } + + // One state for the request, however many tasks it has and whichever request each was given, so completing it + // copies every request its tasks were given at once + private static GivenRequest givenOf(GrailsWebRequest webRequest, HttpServletRequest current) { + // A request a forward dispatched or a filter wrapped shares the attributes of the request it wraps, so its + // tasks find the state of the request, unless the request has completed + GivenRequest given = boundTo(current)?.give(current) + if (given != null) { + return given + } + RequestState state = new RequestState() + given = state.give(current) + if (webRequest.isActive()) { + current.setAttribute(STATE, state) + webRequest.registerDestructionCallback(COMPLETION_CALLBACK, state::requestCompleted, + RequestAttributes.SCOPE_REQUEST) + } + else { + state.requestCompleted() + } + return given + } + + private static RequestState boundTo(HttpServletRequest current) { + try { + Object state = current.getAttribute(STATE) + return state instanceof RequestState ? (RequestState) state : null + } + catch (IllegalStateException ignored) { + // The container has already recycled the request + return null + } + } + + // Every public view of the request and of the requests it wraps, such as a multipart wrapper's, so the task + // finds what a filter resolved however the request is wrapped, as unwrapping the request finds it + private static Class<?>[] interfacesOf(HttpServletRequest request, ClassLoader loader) { + Set<Class<?>> interfaces = new LinkedHashSet<>() + interfaces.add(HttpServletRequest) + for (ServletRequest wrapped = request; wrapped != null; wrapped = unwrap(wrapped)) { + for (Class<?> type = wrapped.getClass(); type != null; type = type.superclass) { + addInterfaces(type.interfaces, loader, interfaces) + } + } + return interfaces.toArray(new Class<?>[0]) + } + + private static void addInterfaces(Class<?>[] candidates, ClassLoader loader, Set<Class<?>> interfaces) { + for (Class<?> candidate : candidates) { + // A proxy class can only implement interfaces it can access, through the class loader that defines it. + // Nor is it a Groovy object, so Groovy calls it through these interfaces, not through the meta class of + // the request's class, whose methods it does not have + if (Modifier.isPublic(candidate.modifiers) && !GroovyObject.isAssignableFrom(candidate) && + visibleFrom(loader, candidate) && interfaces.add(candidate)) { + addInterfaces(candidate.interfaces, loader, interfaces) + } + } + } + + private static boolean visibleFrom(ClassLoader loader, Class<?> type) { + try { + return Class.forName(type.name, false, loader).is(type) + } + catch (ClassNotFoundException ignored) { + return false + } + } + + // The request, or the first request it wraps that offers the method, such as a multipart request in a wrapper + private static ServletRequest offering(ServletRequest request, Class<?> type) { + for (ServletRequest candidate = request; candidate != null; candidate = unwrap(candidate)) { + if (type.isInstance(candidate)) { + return candidate + } + } + return null + } + + private static ServletRequest unwrap(ServletRequest request) { + return request instanceof ServletRequestWrapper ? ((ServletRequestWrapper) request).request : null + } + + @Override + Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (method.name == 'equals' && method.parameterCount == 1) { + return proxy.is(args[0]) + } + if (method.name == 'hashCode' && method.parameterCount == 0) { + return System.identityHashCode(proxy) + } + HttpServletRequest current = given.live + if (current == null) { + current = view() + } + ServletRequest target = offering(current, method.declaringClass) + if (target == null) { + // Only the live request offers the views the copy does not keep, such as a custom wrapper's + throw new IllegalStateException('The request has completed, so a task that outlives it cannot call ' + + "${method.declaringClass.simpleName}.${method.name}, which the copy of the request does not keep") + } + try { + return method.invoke(target, args) + } + catch (InvocationTargetException e) { + throw e.targetException + } + } + + private synchronized HttpServletRequest view() { + if (view == null) { + view = given.copyView() + } + return view + } + + // What the tasks of one request share: its completion, which copies every request they were given at once + private static final class RequestState implements AsyncListener { + + private final List<GivenRequest> given = new ArrayList<>() + private boolean completed + + // The request a task was given, shared by the tasks given the same request, or null once the request has + // completed + private synchronized GivenRequest give(HttpServletRequest request) { + if (completed) { + return null + } + for (GivenRequest existing : given) { + if (existing.live.is(request)) { + return existing + } + } + GivenRequest added = new GivenRequest(request) + given.add(added) + return added + } + + private synchronized HttpServletRequest first() { + return completed ? null : given.get(0).live + } + + private void requestCompleted() { + HttpServletRequest current = first() + if (current == null) { + return + } + try { + if (current.isAsyncStarted()) { + // The response is still being produced, so a task it waits for may still be rendering it + current.asyncContext.addListener(this) + return + } + } + catch (IllegalStateException ignored) { + // Too late to be told when the asynchronous processing completes, so the request is copied now + } + copyRequests() Review Comment: Fixed in f8c3301c99. The copy no longer depends on a web request's completion when the request comes through the container. The async plugin now registers `TaskRequestFilter` as the innermost filter, mapped to the request, async and error dispatches, never to a forward or an include. It marks the request while such a dispatch is processed, and the requests are copied as that dispatch ends, or, while the request is processed asynchronously, as that processing completes. A forward or an include within the dispatch can complete the web request it bound, or the one it reused, and the tasks keep the live request: attributes set and sessions created after the forward returns are there. I kept the copy inside the other filters rather than moving it to the container's `requestDestroyed`. By then Spring Security has cleared its context, so a copy of a request in `SecurityContextHolderAwareRequestWrapper` would have lost the principal. Without the filter, as in a unit test, the web request's completion stays the fallback, as before. A dispatch the filter sees while it already marks the request, such as an error dispatch some containers nest, passes through. Tests: - `a task started during a forward reads the live request until the request that forwarded has been processed`, where the forward binds a web request of its own. - `a task started before a forward that completes the web request it reused reads the live request until the request has been processed`, for a plain forward to the dispatcher servlet. Both create a session from the task after the forward has returned, and see an attribute set after it. Both fail on d7f3923ce9. Two more tests cover the filter itself: `a dispatch within one the filter already marks ... does not end its processing`, and `a request the filter marks that is processed asynchronously is copied once the asynchronous processing completes`. In the pubsub demo, `TaskRequestSpec` runs the same through Tomcat. An action starts a task, forwards with `forward(action: ...)`, and sets an attribute once the forward returns. The tasks read their requests once the request has left the application. ########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/CompletedHttpServletRequest.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.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 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 copies share the attributes and the body, as the + * requests share them, so attributes a task sets stay with the copies, shared by every task. 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>The parameters are those params reads: the text fields of a multipart request an attribute holds, such as the + * one the dispatcher resolved, are added, and the request's own value is kept for a name both have. 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.</p> + * + * <p>The uploads of a multipart request are deleted as the request ends, so the copy answers as a multipart + * request without uploads: through the request of a task when the request was a multipart request, and in place of + * a multipart request an attribute held, such as the one the dispatcher resolved. Reading params then finds the + * copied parameters and no uploads.</p> + * + * @since 8.0.1 + */ +@CompileStatic +class CompletedHttpServletRequest implements MultipartHttpServletRequest { + + private static final byte[] NO_BODY = new byte[0] + 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 byte[] body + 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, + CopiedParameters parameters, byte[] body) { + this.attributes = attributes + this.parameters = parameters.values + this.parameterFailure = parameters.failure + this.body = body + 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 + this.userPrincipal = request.userPrincipal + this.dispatcherType = request.dispatcherType + // Values only: the container's own objects can reference the request it recycles + this.httpServletMapping = copyMapping(request.httpServletMapping) + this.servletConnection = copyConnection(request.servletConnection) + this.authType = request.authType + this.characterEncoding = request.characterEncoding + this.contentType = request.contentType + this.contentLength = request.contentLengthLong + this.method = request.method + this.requestURI = request.requestURI + this.requestURL = request.requestURL?.toString() + this.queryString = request.queryString + this.contextPath = request.contextPath + this.servletPath = request.servletPath + this.pathInfo = request.pathInfo + this.pathTranslated = request.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 = request.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.body = copy.@body + 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 and the body, as the + * requests share them. + * + * @param requests the requests, the first being the one whose attributes and body the copies share + * @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) { + HttpServletRequest first = requests.get(0) + Map<String, Object> attributes = new ConcurrentHashMap<>() + Map<String, MultipartHttpServletRequest> multipartRequests = new LinkedHashMap<>() + for (String name : Collections.list(first.attributeNames)) { Review Comment: Fixed in f8c3301c99. The copies now share the attributes of the request the container created, which every request a task was given shows through. Each copy keeps what its own request shows otherwise: its own values, such as the `jakarta.servlet.forward.*` attributes a forward's request keeps on its wrapper, and the names it hides. Setting or removing one of those changes that copy only, as on the wrapper. So after completion, the forward's copy answers `WebUtils.isForward` with true and `getForwardURI` with the original URI, and the request's copy doesn't get them, whichever task started first. Tomcat's `ApplicationHttpRequest` keeps those attributes in its own fields, and its `recycle()` only ends the session access, so the wrapper still answers when the request that forwarded completes. Checking the other readers of the attributes turned up the same problem in the multipart stand-in. It was the first copy, so when the first task was given the forward, `params` of the request's copy merged the forward's parameters. A copy of the multipart request itself now takes its place, with its own parameters, so `params` on each copy merges what it merged on the live request. Each copy's own parameters are now its request's alone, as on the live request. The text fields only the dispatcher's multipart request has still reach `params` through that request's copy. Tests: - `each copy keeps the forward attributes its own request shows`, run in both orders. - `a task changes an attribute a forward keeps itself on the copy of the forward only, as on the live request`, which also checks that a forward hides the include attribute of the request underneath. - `params of each copy reads what it read on the live request`, run in both orders. All fail on d7f3923ce9, except the `params` case where the request's task starts first, which the old stand-in already served. -- 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]
