This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 52ba355639 Minor fixes from code review
52ba355639 is described below
commit 52ba3556391467d0c3adf3bcde09f32382ec57b9
Author: remm <[email protected]>
AuthorDate: Mon Jun 22 15:44:27 2026 +0200
Minor fixes from code review
Most significant is the rework of the wrapper unload.
---
.../catalina/core/ApplicationDispatcher.java | 3 ---
.../catalina/core/ApplicationHttpRequest.java | 5 ++--
.../catalina/core/ApplicationHttpResponse.java | 2 +-
.../apache/catalina/core/ApplicationRequest.java | 2 +-
.../apache/catalina/core/ApplicationResponse.java | 2 +-
.../core/ApplicationServletRegistration.java | 1 +
.../org/apache/catalina/core/AsyncContextImpl.java | 2 +-
.../core/JreMemoryLeakPreventionListener.java | 2 +-
.../catalina/core/NamingContextListener.java | 15 ++++++-----
.../apache/catalina/core/StandardEngineValve.java | 3 +--
java/org/apache/catalina/core/StandardHost.java | 4 +--
.../org/apache/catalina/core/StandardPipeline.java | 1 +
.../core/StandardVirtualThreadExecutor.java | 5 +++-
java/org/apache/catalina/core/StandardWrapper.java | 14 ++++++----
.../catalina/core/StandardWrapperFacade.java | 4 +--
.../core/ThreadLocalLeakPreventionListener.java | 8 +++---
.../catalina/deploy/NamingResourcesImpl.java | 31 +++++++++++-----------
java/org/apache/catalina/filters/CorsFilter.java | 2 +-
18 files changed, 59 insertions(+), 47 deletions(-)
diff --git a/java/org/apache/catalina/core/ApplicationDispatcher.java
b/java/org/apache/catalina/core/ApplicationDispatcher.java
index 5c257619e6..8cb78131dc 100644
--- a/java/org/apache/catalina/core/ApplicationDispatcher.java
+++ b/java/org/apache/catalina/core/ApplicationDispatcher.java
@@ -450,9 +450,6 @@ final class ApplicationDispatcher implements
AsyncDispatcher, RequestDispatcher
/**
* 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
index 24d7f51d43..de9c20bebc 100644
--- a/java/org/apache/catalina/core/ApplicationHttpRequest.java
+++ b/java/org/apache/catalina/core/ApplicationHttpRequest.java
@@ -54,7 +54,7 @@ import org.apache.tomcat.util.res.StringManager;
* 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 @@ class ApplicationHttpRequest extends
HttpServletRequestWrapper {
// -------------------------------------------------------- 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
index 7b397dbd5c..68c27e4e40 100644
--- a/java/org/apache/catalina/core/ApplicationHttpResponse.java
+++ b/java/org/apache/catalina/core/ApplicationHttpResponse.java
@@ -28,7 +28,7 @@ import jakarta.servlet.http.HttpServletResponseWrapper;
* 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
index f082455adc..b8ca244a1d 100644
--- a/java/org/apache/catalina/core/ApplicationRequest.java
+++ b/java/org/apache/catalina/core/ApplicationRequest.java
@@ -33,7 +33,7 @@ import org.apache.tomcat.util.res.StringManager;
* 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
index c2e91370e2..1c2944b477 100644
--- a/java/org/apache/catalina/core/ApplicationResponse.java
+++ b/java/org/apache/catalina/core/ApplicationResponse.java
@@ -26,7 +26,7 @@ import jakarta.servlet.ServletResponseWrapper;
* 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
index 3433932abb..93a2c47506 100644
--- a/java/org/apache/catalina/core/ApplicationServletRegistration.java
+++ b/java/org/apache/catalina/core/ApplicationServletRegistration.java
@@ -196,6 +196,7 @@ public class ApplicationServletRegistration implements
ServletRegistration.Dynam
*/
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
index f70bd2ef2f..9915d0b1e5 100644
--- a/java/org/apache/catalina/core/AsyncContextImpl.java
+++ b/java/org/apache/catalina/core/AsyncContextImpl.java
@@ -644,7 +644,7 @@ public class AsyncContextImpl implements AsyncContext,
AsyncContextCallback {
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
index 554d7e2c8a..de219d11d5 100644
--- a/java/org/apache/catalina/core/JreMemoryLeakPreventionListener.java
+++ b/java/org/apache/catalina/core/JreMemoryLeakPreventionListener.java
@@ -238,7 +238,7 @@ public class JreMemoryLeakPreventionListener implements
LifecycleListener {
// 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
index 27f9711a84..8ea24c293c 100644
--- a/java/org/apache/catalina/core/NamingContextListener.java
+++ b/java/org/apache/catalina/core/NamingContextListener.java
@@ -37,13 +37,14 @@ import javax.naming.NamingException;
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 class NamingContextListener implements
LifecycleListener, PropertyChangeL
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
index 546c8d46ac..cf7ea22f82 100644
--- a/java/org/apache/catalina/core/StandardEngineValve.java
+++ b/java/org/apache/catalina/core/StandardEngineValve.java
@@ -56,8 +56,7 @@ final class StandardEngineValve extends ValveBase {
// 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
index daaca98633..fbb55dfab9 100644
--- a/java/org/apache/catalina/core/StandardHost.java
+++ b/java/org/apache/catalina/core/StandardHost.java
@@ -502,9 +502,9 @@ public class StandardHost extends ContainerBase implements
Host {
*/
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
index 29fa0e5197..ca640db4ff 100644
--- a/java/org/apache/catalina/core/StandardPipeline.java
+++ b/java/org/apache/catalina/core/StandardPipeline.java
@@ -365,6 +365,7 @@ public class StandardPipeline extends LifecycleBase
implements Pipeline {
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
index faa652b0f1..932f439590 100644
--- a/java/org/apache/catalina/core/StandardVirtualThreadExecutor.java
+++ b/java/org/apache/catalina/core/StandardVirtualThreadExecutor.java
@@ -99,8 +99,11 @@ public class StandardVirtualThreadExecutor extends
LifecycleMBeanBase implements
@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
index 3575ef79b8..4121a4e582 100644
--- a/java/org/apache/catalina/core/StandardWrapper.java
+++ b/java/org/apache/catalina/core/StandardWrapper.java
@@ -936,6 +936,8 @@ public class StandardWrapper extends ContainerBase
implements ServletConfig, Wra
}
}
+ ServletException servletException = null;
+
if (instanceInitialized) {
PrintStream out = System.out;
if (swallowOutput) {
@@ -948,9 +950,7 @@ public class StandardWrapper extends ContainerBase
implements ServletConfig, Wra
} 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 class StandardWrapper extends ContainerBase
implements ServletConfig, Wra
}
}
}
- instance = null;
instanceInitialized = false;
}
}
@@ -986,6 +985,11 @@ public class StandardWrapper extends ContainerBase
implements ServletConfig, Wra
unloading = false;
fireContainerEvent("unload", this);
+
+ if (servletException != null) {
+ throw servletException;
+ }
+
}
@@ -1324,7 +1328,7 @@ public class StandardWrapper extends ContainerBase
implements ServletConfig, Wra
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
index 1128fea374..4432f99280 100644
--- a/java/org/apache/catalina/core/StandardWrapperFacade.java
+++ b/java/org/apache/catalina/core/StandardWrapperFacade.java
@@ -72,8 +72,8 @@ public final class StandardWrapperFacade implements
ServletConfig {
@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
index bc86774b6c..836e3686d9 100644
--- a/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
+++ b/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
@@ -18,9 +18,9 @@ package org.apache.catalina.core;
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 @@ public class ThreadLocalLeakPreventionListener extends
FrameworkListener {
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
index ff0e38bf3e..850cc66acf 100644
--- a/java/org/apache/catalina/deploy/NamingResourcesImpl.java
+++ b/java/org/apache/catalina/deploy/NamingResourcesImpl.java
@@ -24,20 +24,20 @@ import java.lang.reflect.Field;
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 class NamingResourcesImpl extends LifecycleMBeanBase
implements Serializa
/**
* 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 class NamingResourcesImpl extends LifecycleMBeanBase
implements Serializa
/**
* 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 class NamingResourcesImpl extends
LifecycleMBeanBase implements Serializa
}
// 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,7 +912,7 @@ public class NamingResourcesImpl extends LifecycleMBeanBase
implements Serializa
// 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) {
@@ -919,7 +920,7 @@ public class NamingResourcesImpl extends LifecycleMBeanBase
implements Serializa
}
}
- for (ContextEnvironment ce : envs.values()) {
+ for (ContextEnvironment ce : findEnvironments()) {
try {
MBeanUtils.createMBean(ce);
} catch (Exception e) {
@@ -927,7 +928,7 @@ public class NamingResourcesImpl extends LifecycleMBeanBase
implements Serializa
}
}
- for (ContextResourceLink crl : resourceLinks.values()) {
+ for (ContextResourceLink crl : findResourceLinks()) {
try {
MBeanUtils.createMBean(crl);
} catch (Exception e) {
@@ -970,7 +971,7 @@ public class NamingResourcesImpl extends LifecycleMBeanBase
implements Serializa
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,7 +1027,7 @@ public class NamingResourcesImpl extends
LifecycleMBeanBase implements Serializa
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) {
@@ -1034,7 +1035,7 @@ public class NamingResourcesImpl extends
LifecycleMBeanBase implements Serializa
}
}
- for (ContextEnvironment ce : envs.values()) {
+ for (ContextEnvironment ce : findEnvironments()) {
try {
MBeanUtils.destroyMBean(ce);
} catch (Exception e) {
@@ -1042,7 +1043,7 @@ public class NamingResourcesImpl extends
LifecycleMBeanBase implements Serializa
}
}
- 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
index 7e0c8dca74..52be34ee19 100644
--- a/java/org/apache/catalina/filters/CorsFilter.java
+++ b/java/org/apache/catalina/filters/CorsFilter.java
@@ -73,7 +73,7 @@ import org.apache.tomcat.util.res.StringManager;
* {@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 {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
