Minor fixes from code review

rmaucher · rmaucher · commit 52ba35563914 · 2026-06-22T15:44:27.000+02:00
Most significant is the rework of the wrapper unload.
diff --git a/java/org/apache/catalina/core/ApplicationDispatcher.java b/java/org/apache/catalina/core/ApplicationDispatcher.java
@@ -450,9 +450,6 @@ public void dispatch(ServletRequest request, ServletResponse response) throws Se
     /**
      * Ask the resource represented by this RequestDispatcher to process the associated request, and create (or append
      * to) the associated response.
-     * <p>
-     * <strong>IMPLEMENTATION NOTE</strong>: This implementation assumes that no filters are applied to a forwarded or
-     * included resource, because they were already done for the original request.
      *
      * @param request  The servlet request we are processing
      * @param response The servlet response we are creating
diff --git a/java/org/apache/catalina/core/ApplicationHttpRequest.java b/java/org/apache/catalina/core/ApplicationHttpRequest.java
@@ -54,7 +54,7 @@
  * Wrapper around a <code>jakarta.servlet.http.HttpServletRequest</code> that transforms an application request object
  * (which might be the original one passed to a servlet, or might be based on the 2.3
  * <code>jakarta.servlet.http.HttpServletRequestWrapper</code> class) back into an internal
- * <code>org.apache.catalina.HttpRequest</code>.
+ * <code>org.apache.catalina.connector.Request</code>.
  * <p>
  * <strong>WARNING</strong>: Due to Java's lack of support for multiple inheritance, all of the logic in
  * <code>ApplicationRequest</code> is duplicated in <code>ApplicationHttpRequest</code>. Make sure that you keep these
@@ -621,7 +621,8 @@ public boolean isRequestedSessionIdValid() {
     // -------------------------------------------------------- Package Methods
 
     /**
-     * Recycle this request
+     * Recycle this request. Since there is no object reuse, this only ends
+     * the session access.
      */
     public void recycle() {
         if (session != null) {
diff --git a/java/org/apache/catalina/core/ApplicationHttpResponse.java b/java/org/apache/catalina/core/ApplicationHttpResponse.java
@@ -28,7 +28,7 @@
  * Wrapper around a <code>jakarta.servlet.http.HttpServletResponse</code> that transforms an application response object
  * (which might be the original one passed to a servlet, or might be based on the 2.3
  * <code>jakarta.servlet.http.HttpServletResponseWrapper</code> class) back into an internal
- * <code>org.apache.catalina.HttpResponse</code>.
+ * <code>org.apache.catalina.connector.Response</code>.
  * <p>
  * <strong>WARNING</strong>: Due to Java's lack of support for multiple inheritance, all of the logic in
  * <code>ApplicationResponse</code> is duplicated in <code>ApplicationHttpResponse</code>. Make sure that you keep these
diff --git a/java/org/apache/catalina/core/ApplicationRequest.java b/java/org/apache/catalina/core/ApplicationRequest.java
@@ -33,7 +33,7 @@
  * Wrapper around a <code>jakarta.servlet.ServletRequest</code> that transforms an application request object (which
  * might be the original one passed to a servlet, or might be based on the 2.3
  * <code>jakarta.servlet.ServletRequestWrapper</code> class) back into an internal
- * <code>org.apache.catalina.Request</code>.
+ * <code>org.apache.catalina.connector.Request</code>.
  * <p>
  * <strong>WARNING</strong>: Due to Java's lack of support for multiple inheritance, all of the logic in
  * <code>ApplicationRequest</code> is duplicated in <code>ApplicationHttpRequest</code>. Make sure that you keep these
diff --git a/java/org/apache/catalina/core/ApplicationResponse.java b/java/org/apache/catalina/core/ApplicationResponse.java
@@ -26,7 +26,7 @@
  * Wrapper around a <code>jakarta.servlet.ServletResponse</code> that transforms an application response object (which
  * might be the original one passed to a servlet, or might be based on the 2.3
  * <code>jakarta.servlet.ServletResponseWrapper</code> class) back into an internal
- * <code>org.apache.catalina.Response</code>.
+ * <code>org.apache.catalina.connector.Response</code>.
  * <p>
  * <strong>WARNING</strong>: Due to Java's lack of support for multiple inheritance, all of the logic in
  * <code>ApplicationResponse</code> is duplicated in <code>ApplicationHttpResponse</code>. Make sure that you keep these
diff --git a/java/org/apache/catalina/core/ApplicationServletRegistration.java b/java/org/apache/catalina/core/ApplicationServletRegistration.java
@@ -196,6 +196,7 @@ public Set<String> addMapping(String... urlPatterns) {
                      */
                     overrides.add(decodedUrlPatterns[i]);
                 } else {
+                    // The conflicts list the original URL patterns passed
                     conflicts.add(urlPatterns[i]);
                 }
             }
diff --git a/java/org/apache/catalina/core/AsyncContextImpl.java b/java/org/apache/catalina/core/AsyncContextImpl.java
@@ -644,7 +644,7 @@ private void logDebug(String method) {
 
     private void check() {
         Request request = this.request;
-        if (hasBeenRecycled.get()) {
+        if (hasBeenRecycled.get() || request == null) {
             // AsyncContext has been recycled and should not be being used
             throw new IllegalStateException(sm.getString("asyncContextImpl.requestEnded"));
         }
diff --git a/java/org/apache/catalina/core/JreMemoryLeakPreventionListener.java b/java/org/apache/catalina/core/JreMemoryLeakPreventionListener.java
@@ -238,7 +238,7 @@ public void lifecycleEvent(LifecycleEvent event) {
 
                 // Set the default URL caching policy to not to cache
                 if (urlCacheProtection) {
-                    URLConnection.setDefaultUseCaches("JAR", false);
+                    URLConnection.setDefaultUseCaches("jar", false);
                 }
 
                 /*
diff --git a/java/org/apache/catalina/core/NamingContextListener.java b/java/org/apache/catalina/core/NamingContextListener.java
@@ -37,13 +37,14 @@
 import javax.naming.Reference;
 import javax.naming.StringRefAddr;
 
+import org.apache.catalina.Container;
 import org.apache.catalina.Context;
-import org.apache.catalina.Engine;
 import org.apache.catalina.Host;
 import org.apache.catalina.Lifecycle;
 import org.apache.catalina.LifecycleEvent;
 import org.apache.catalina.LifecycleListener;
 import org.apache.catalina.Server;
+import org.apache.catalina.Service;
 import org.apache.catalina.deploy.NamingResourcesImpl;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
@@ -1083,11 +1084,13 @@ public void addResourceLink(ContextResourceLink resourceLink) {
 
     private javax.naming.Context getGlobalNamingContext() {
         if (container instanceof Context) {
-            Engine e = (Engine) ((Context) container).getParent().getParent();
-            Server s = e.getService().getServer();
-            // When the Service is an embedded Service, there is no Server
-            if (s != null) {
-                return s.getGlobalNamingContext();
+            Service service = Container.getService((Context) container);
+            if (service != null) {
+                Server s = service.getServer();
+                // When the Service is an embedded Service, there is no Server
+                if (s != null) {
+                    return s.getGlobalNamingContext();
+                }
             }
         }
         return null;
diff --git a/java/org/apache/catalina/core/StandardEngineValve.java b/java/org/apache/catalina/core/StandardEngineValve.java
@@ -56,8 +56,7 @@ public void invoke(Request request, Response response) throws IOException, Servl
         // Select the Host to be used for this Request
         Host host = request.getHost();
         if (host == null) {
-            // HTTP 0.9 or HTTP 1.0 request without a host when no default host
-            // is defined.
+            // No Host found for this request (unknown server name or no default host defined)
             // Don't overwrite an existing error
             if (!response.isError()) {
                 response.sendError(404);
diff --git a/java/org/apache/catalina/core/StandardHost.java b/java/org/apache/catalina/core/StandardHost.java
@@ -502,9 +502,9 @@ public String getErrorReportValveClass() {
      */
     public void setErrorReportValveClass(String errorReportValveClass) {
 
-        String oldErrorReportValveClassClass = this.errorReportValveClass;
+        String oldErrorReportValveClass = this.errorReportValveClass;
         this.errorReportValveClass = errorReportValveClass;
-        support.firePropertyChange("errorReportValveClass", oldErrorReportValveClassClass, this.errorReportValveClass);
+        support.firePropertyChange("errorReportValveClass", oldErrorReportValveClass, this.errorReportValveClass);
 
     }
 
diff --git a/java/org/apache/catalina/core/StandardPipeline.java b/java/org/apache/catalina/core/StandardPipeline.java
@@ -365,6 +365,7 @@ public void removeValve(Valve valve) {
         if (first == basic) {
             first = null;
         }
+        // Note: Removing the basic valve is done by replacing it using setBasic
 
         cleanupValve(valve);
 
diff --git a/java/org/apache/catalina/core/StandardVirtualThreadExecutor.java b/java/org/apache/catalina/core/StandardVirtualThreadExecutor.java
@@ -99,8 +99,11 @@ protected void startInternal() throws LifecycleException {
 
     @Override
     protected void stopInternal() throws LifecycleException {
-        executor = null;
         setState(LifecycleState.STOPPING);
+        if (executor != null) {
+            executor.shutdownNow();
+        }
+        executor = null;
     }
 
     @Override
diff --git a/java/org/apache/catalina/core/StandardWrapper.java b/java/org/apache/catalina/core/StandardWrapper.java
@@ -936,6 +936,8 @@ public synchronized void unload() throws ServletException {
             }
         }
 
+        ServletException servletException = null;
+
         if (instanceInitialized) {
             PrintStream out = System.out;
             if (swallowOutput) {
@@ -948,9 +950,7 @@ public synchronized void unload() throws ServletException {
             } catch (Throwable t) {
                 Throwable throwable = ExceptionUtils.unwrapInvocationTargetException(t);
                 ExceptionUtils.handleThrowable(throwable);
-                fireContainerEvent("unload", this);
-                unloading = false;
-                throw new ServletException(sm.getString("standardWrapper.destroyException", getName()), throwable);
+                servletException = new ServletException(sm.getString("standardWrapper.destroyException", getName()), throwable);
             } finally {
                 // Annotation processing
                 if (!((Context) getParent()).getIgnoreAnnotations()) {
@@ -972,7 +972,6 @@ public synchronized void unload() throws ServletException {
                         }
                     }
                 }
-                instance = null;
                 instanceInitialized = false;
             }
         }
@@ -986,6 +985,11 @@ public synchronized void unload() throws ServletException {
 
         unloading = false;
         fireContainerEvent("unload", this);
+
+        if (servletException != null) {
+            throw servletException;
+        }
+
     }
 
 
@@ -1324,7 +1328,7 @@ public MBeanNotificationInfo[] getNotificationInfo() {
                     new MBeanNotificationInfo(new String[] { "j2ee.state.running" }, Notification.class.getName(),
                             "servlet is running"),
                     new MBeanNotificationInfo(new String[] { "j2ee.state.stopped" }, Notification.class.getName(),
-                            "servlet start to stopped"),
+                            "servlet is stopping"),
                     new MBeanNotificationInfo(new String[] { "j2ee.object.stopped" }, Notification.class.getName(),
                             "servlet is stopped"),
                     new MBeanNotificationInfo(new String[] { "j2ee.object.deleted" }, Notification.class.getName(),
diff --git a/java/org/apache/catalina/core/StandardWrapperFacade.java b/java/org/apache/catalina/core/StandardWrapperFacade.java
@@ -72,8 +72,8 @@ public String getServletName() {
     @Override
     public ServletContext getServletContext() {
         /*
-         * This method may be called concurrently but the same context object will always be returned. There is no
-         * concurrency issue here.
+         * Multiple threads may initialize context concurrently, but the same context/facade
+         * object is always returned, so this is functionally safe.
          */
         if (context == null) {
             context = config.getServletContext();
diff --git a/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java b/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
@@ -18,9 +18,9 @@
 
 import java.util.concurrent.Executor;
 
+import org.apache.catalina.Container;
 import org.apache.catalina.ContainerEvent;
 import org.apache.catalina.Context;
-import org.apache.catalina.Engine;
 import org.apache.catalina.Lifecycle;
 import org.apache.catalina.LifecycleEvent;
 import org.apache.catalina.LifecycleListener;
@@ -111,8 +111,10 @@ private void stopIdleThreads(Context context) {
             return;
         }
 
-        Engine engine = (Engine) context.getParent().getParent();
-        Service service = engine.getService();
+        Service service = Container.getService(context);
+        if (service == null) {
+            return;
+        }
         Connector[] connectors = service.findConnectors();
         if (connectors != null) {
             for (Connector connector : connectors) {
diff --git a/java/org/apache/catalina/deploy/NamingResourcesImpl.java b/java/org/apache/catalina/deploy/NamingResourcesImpl.java
@@ -24,20 +24,20 @@
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import javax.naming.NamingException;
 
 import org.apache.catalina.Container;
 import org.apache.catalina.Context;
-import org.apache.catalina.Engine;
 import org.apache.catalina.JmxEnabled;
 import org.apache.catalina.LifecycleException;
 import org.apache.catalina.LifecycleState;
 import org.apache.catalina.Server;
+import org.apache.catalina.Service;
 import org.apache.catalina.mbeans.MBeanUtils;
 import org.apache.catalina.util.Introspection;
 import org.apache.catalina.util.LifecycleMBeanBase;
@@ -100,7 +100,7 @@ public NamingResourcesImpl() {
     /**
      * Set of naming entries, keyed by name.
      */
-    private final Set<String> entries = new HashSet<>();
+    private final Set<String> entries = ConcurrentHashMap.newKeySet();
 
 
     /**
@@ -175,7 +175,7 @@ public Object getContainer() {
     /**
      * Set the container with which the naming resources are associated.
      *
-     * @param container the associated with the resources
+     * @param container the container associated with the resources
      */
     public void setContainer(Object container) {
         this.container = container;
@@ -303,15 +303,16 @@ public void addEnvironment(ContextEnvironment environment) {
     }
 
     // Container should be an instance of Server or Context. If it is anything
-    // else, return null which will trigger a NPE.
+    // else, return null.
     private Server getServer() {
         if (container instanceof Server) {
             return (Server) container;
         }
         if (container instanceof Context) {
-            // Could do this in one go. Lots of casts so split out for clarity
-            Engine engine = (Engine) ((Context) container).getParent().getParent();
-            return engine.getService().getServer();
+            Service service = Container.getService((Context) container);
+            if (service != null) {
+                return service.getServer();
+            }
         }
         return null;
     }
@@ -911,23 +912,23 @@ protected void initInternal() throws LifecycleException {
         // timing issues. Duplication registration is not an issue.
         resourceRequireExplicitRegistration = true;
 
-        for (ContextResource cr : resources.values()) {
+        for (ContextResource cr : findResources()) {
             try {
                 MBeanUtils.createMBean(cr);
             } catch (Exception e) {
                 log.warn(sm.getString("namingResources.mbeanCreateFail", cr.getName()), e);
             }
         }
 
-        for (ContextEnvironment ce : envs.values()) {
+        for (ContextEnvironment ce : findEnvironments()) {
             try {
                 MBeanUtils.createMBean(ce);
             } catch (Exception e) {
                 log.warn(sm.getString("namingResources.mbeanCreateFail", ce.getName()), e);
             }
         }
 
-        for (ContextResourceLink crl : resourceLinks.values()) {
+        for (ContextResourceLink crl : findResourceLinks()) {
             try {
                 MBeanUtils.createMBean(crl);
             } catch (Exception e) {
@@ -970,7 +971,7 @@ private void cleanUp() {
             log.warn(sm.getString("namingResources.cleanupNoContext", container), e);
             return;
         }
-        for (ContextResource cr : resources.values()) {
+        for (ContextResource cr : findResources()) {
             if (cr.getSingleton()) {
                 String closeMethod = cr.getCloseMethod();
                 if (closeMethod != null && !closeMethod.isEmpty()) {
@@ -1026,23 +1027,23 @@ protected void destroyInternal() throws LifecycleException {
         resourceRequireExplicitRegistration = false;
 
         // Destroy in reverse order to create, although it should not matter
-        for (ContextResourceLink crl : resourceLinks.values()) {
+        for (ContextResourceLink crl : findResourceLinks()) {
             try {
                 MBeanUtils.destroyMBean(crl);
             } catch (Exception e) {
                 log.warn(sm.getString("namingResources.mbeanDestroyFail", crl.getName()), e);
             }
         }
 
-        for (ContextEnvironment ce : envs.values()) {
+        for (ContextEnvironment ce : findEnvironments()) {
             try {
                 MBeanUtils.destroyMBean(ce);
             } catch (Exception e) {
                 log.warn(sm.getString("namingResources.mbeanDestroyFail", ce.getName()), e);
             }
         }
 
-        for (ContextResource cr : resources.values()) {
+        for (ContextResource cr : findResources()) {
             try {
                 MBeanUtils.destroyMBean(cr);
             } catch (Exception e) {
diff --git a/java/org/apache/catalina/filters/CorsFilter.java b/java/org/apache/catalina/filters/CorsFilter.java
@@ -73,7 +73,7 @@
  * {@link CorsFilter#doFilter(ServletRequest, ServletResponse, FilterChain)} and add appropriate locking so that the
  * {@code doFilter()} method executes with a consistent configuration.
  *
- * @see <a href="http://www.w3.org/TR/cors/">CORS specification</a>
+ * @see <a href="https://fetch.spec.whatwg.org/#http-cors-protocol">CORS specification</a>
  */
 public class CorsFilter extends GenericFilter {
 

Original file line number	Diff line number	Diff line change
`@@ -196,6 +196,7 @@ public Set<String> addMapping(String... urlPatterns) {`
`196`	`196`	`*/`
`197`	`197`	`overrides.add(decodedUrlPatterns[i]);`
`198`	`198`	`} else {`
	`199`	`+ // The conflicts list the original URL patterns passed`
`199`	`200`	`conflicts.add(urlPatterns[i]);`
`200`	`201`	`}`
`201`	`202`	`}`
Original file line number	Diff line number	Diff line change
`@@ -644,7 +644,7 @@ private void logDebug(String method) {`
`644`	`644`
`645`	`645`	`private void check() {`
`646`	`646`	`Request request = this.request;`
`647`		`- if (hasBeenRecycled.get()) {`
	`647`	`+ if (hasBeenRecycled.get() \|\| request == null) {`
`648`	`648`	`// AsyncContext has been recycled and should not be being used`
`649`	`649`	`throw new IllegalStateException(sm.getString("asyncContextImpl.requestEnded"));`
`650`	`650`	`}`
Original file line number	Diff line number	Diff line change
`@@ -238,7 +238,7 @@ public void lifecycleEvent(LifecycleEvent event) {`
`238`	`238`
`239`	`239`	`// Set the default URL caching policy to not to cache`
`240`	`240`	`if (urlCacheProtection) {`
`241`		`- URLConnection.setDefaultUseCaches("JAR", false);`
	`241`	`+ URLConnection.setDefaultUseCaches("jar", false);`
`242`	`242`	`}`
`243`	`243`
`244`	`244`	`/*`