hk2166 commented on code in PR #16786: URL: https://github.com/apache/grails-core/pull/16786#discussion_r4401981146
########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/TaskRequest.groovy: ########## @@ -0,0 +1,237 @@ +/* + * 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 jakarta.servlet.AsyncEvent +import jakarta.servlet.AsyncListener +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 that request, with every public view the request offers, such as a + * multipart wrapper's, so a task the response waits for, such as one started with {@code WebPromises}, renders + * through it. The container recycles the request once it completes, including any asynchronous processing, so + * from then on it is a {@link CompletedHttpServletRequest copy} of the request taken as it completed, and the + * recycled request is released. One copy is taken for the request, however many tasks it has, and each task + * reads it through a view of its own, so each has the body to read.</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 RequestState state + private CompletedHttpServletRequest view + + private TaskRequest(RequestState state) { + this.state = state + } + + /** + * 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 same request through a view of its own + return (HttpServletRequest) Proxy.newProxyInstance(current.getClass().classLoader, + current.getClass().interfaces, new TaskRequest(((TaskRequest) handler).state)) + } + } + ClassLoader loader = current.getClass().classLoader ?: TaskRequest.classLoader + return (HttpServletRequest) Proxy.newProxyInstance(loader, interfacesOf(current), + new TaskRequest(stateOf(webRequest, current))) + } + + // One state for the request, however many tasks it has, so completing it takes the one copy they all read + private static RequestState stateOf(GrailsWebRequest webRequest, HttpServletRequest current) { + RequestState existing = boundTo(current) + if (existing != null) { + return existing + } + RequestState state = new RequestState(current) + if (webRequest.isActive()) { + current.setAttribute(STATE, state) + webRequest.registerDestructionCallback(COMPLETION_CALLBACK, state::requestCompleted, + RequestAttributes.SCOPE_REQUEST) + } + else { + state.requestCompleted() + } + return state + } + + 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, such as a multipart wrapper's, so the task finds what a filter resolved + private static Class<?>[] interfacesOf(HttpServletRequest request) { + Set<Class<?>> interfaces = new LinkedHashSet<>() + interfaces.add(HttpServletRequest) + for (Class<?> type = request.getClass(); type != null; type = type.superclass) { Review Comment: The task's request now takes the public interfaces of every request in the wrapper chain, not only those of the outer class, and `invoke` sends each call to the first request in the chain that offers the method, so `getMultiFileMap` reaches the multipart request inside the wrapper, and `resolveMultipartRequest` and `params` find the upload while the request is live. Since the interfaces now come from several classes, only those the class loader defining the proxy can see are taken: a proxy class cannot implement any other, and `Proxy.newProxyInstance` would throw. Covered by `a task finds a multipart request inside an ordinary wrapper, as params does on the request itself`, and for the class loader by `a task starts for a request wrapping one with an interface the class loader of the request cannot see` (without the visibility check, starting the task throws `IllegalArgumentException: ... is not visible from class loader`). ########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/TaskRequest.groovy: ########## @@ -0,0 +1,237 @@ +/* + * 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 jakarta.servlet.AsyncEvent +import jakarta.servlet.AsyncListener +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 that request, with every public view the request offers, such as a + * multipart wrapper's, so a task the response waits for, such as one started with {@code WebPromises}, renders + * through it. The container recycles the request once it completes, including any asynchronous processing, so + * from then on it is a {@link CompletedHttpServletRequest copy} of the request taken as it completed, and the + * recycled request is released. One copy is taken for the request, however many tasks it has, and each task + * reads it through a view of its own, so each has the body to read.</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 RequestState state + private CompletedHttpServletRequest view + + private TaskRequest(RequestState state) { + this.state = state + } + + /** + * 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 same request through a view of its own + return (HttpServletRequest) Proxy.newProxyInstance(current.getClass().classLoader, + current.getClass().interfaces, new TaskRequest(((TaskRequest) handler).state)) + } + } + ClassLoader loader = current.getClass().classLoader ?: TaskRequest.classLoader + return (HttpServletRequest) Proxy.newProxyInstance(loader, interfacesOf(current), + new TaskRequest(stateOf(webRequest, current))) + } + + // One state for the request, however many tasks it has, so completing it takes the one copy they all read + private static RequestState stateOf(GrailsWebRequest webRequest, HttpServletRequest current) { + RequestState existing = boundTo(current) + if (existing != null) { + return existing + } + RequestState state = new RequestState(current) + if (webRequest.isActive()) { + current.setAttribute(STATE, state) + webRequest.registerDestructionCallback(COMPLETION_CALLBACK, state::requestCompleted, + RequestAttributes.SCOPE_REQUEST) + } + else { + state.requestCompleted() + } + return state + } + + 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, such as a multipart wrapper's, so the task finds what a filter resolved + private static Class<?>[] interfacesOf(HttpServletRequest request) { + Set<Class<?>> interfaces = new LinkedHashSet<>() + interfaces.add(HttpServletRequest) + for (Class<?> type = request.getClass(); type != null; type = type.superclass) { + addInterfaces(type.interfaces, interfaces) + } + return interfaces.toArray(new Class<?>[0]) + } + + private static void addInterfaces(Class<?>[] candidates, Set<Class<?>> interfaces) { + for (Class<?> candidate : candidates) { + // A proxy class can only implement interfaces it can access + if (Modifier.isPublic(candidate.modifiers) && interfaces.add(candidate)) { + addInterfaces(candidate.interfaces, interfaces) + } + } + } + + @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 = state.live + if (current == null) { + current = view() + } + if (!method.declaringClass.isInstance(current)) { Review Comment: The copy now implements `MultipartHttpServletRequest` and answers as a multipart request without uploads: `getMultiFileMap`, `getFileMap`, `getFiles` and `getFileNames` are empty, `getFile`, `getMultipartContentType` and `getMultipartHeaders` return `null`, and `getRequestMethod` and `getRequestHeaders` come from the copy. I went with empty rather than copied files because the uploads are gone by the time the copy is taken: Spring cleans up a resolved multipart request when its dispatch ends, before the request completes, and the container deletes the parts when it recycles the request. So reading `params` after completion gives the copied parameters and no files. Covered by `a multipart request stays multipart for a task, and once the request completes it has no uploads` and by the lazy-multipart test, which both threw on `getMultiFileMap` before. ########## grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/CompletedHttpServletRequest.groovy: ########## @@ -0,0 +1,735 @@ +/* + * 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.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.util.LinkedCaseInsensitiveMap + +/** + * 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. The copy holds the attributes, parameters, headers, locales and cookies of the request, the part + * of its body nothing had read, its session and what identifies it. Attributes a task sets stay with the copy. + * What only the live request can do, such as dispatching, processing asynchronously, creating a session or + * checking a role, is not available.</p> + * + * @since 8.0.1 + */ +@CompileStatic +class CompletedHttpServletRequest implements HttpServletRequest { + + 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 = new ConcurrentHashMap<>() + private final byte[] body + private final Map<String, String[]> parameters + 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 CompletedHttpServletRequest(HttpServletRequest request) { + for (String name : Collections.list(request.attributeNames)) { + Object value = request.getAttribute(name) + if (value != null) { + attributes.put(name, value) Review Comment: Right, the identity check missed a wrapper of the request. The copy now leaves out every attribute holding a request (the request itself or any wrapper of it), a response, or a web request (`RequestAttributes`, which covers `org.grails.WEB_REQUEST`), since all of them are recycled with the request. A multipart request that an attribute held, such as the one `GrailsDispatcherServlet.checkMultipart` publishes, is replaced by the copy itself, which has no uploads either. So after completion `resolveMultipartRequest` still finds a multipart request, and `getFile` returns `null` instead of reporting that multipart support is not enabled. The release test now also puts the dispatcher's wrapper and the response on the request and still sees the request collected, and `the copy stands in for the multipart request the dispatcher resolved, without its uploads` covers the attribute. -- 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]
