Remove Jetty specific ServletHolder from the AdditionalServlet interface

lhotari · lhotari · commit de6e02b51cf0 · 2025-12-23T15:18:09.000+02:00
diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServlet.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServlet.java
@@ -20,13 +20,19 @@
 
 import com.google.common.annotations.Beta;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 
 /**
  * The additional servlet interface for support additional servlet.
  */
 @Beta
 public interface AdditionalServlet extends AutoCloseable {
+    /**
+     * The servlet implementation type enum.
+     * Currently, only {@link AdditionalServletType#JAVAX_SERVLET} is supported.
+     */
+    enum AdditionalServletType {
+        JAVAX_SERVLET
+    }
 
     /**
      * load plugin config.
@@ -43,11 +49,24 @@ public interface AdditionalServlet extends AutoCloseable {
     String getBasePath();
 
     /**
-     * Get the servlet holder.
+     * Get the servlet type that this implementation uses.
+     */
+    default AdditionalServletType getServletType() {
+        return AdditionalServletType.JAVAX_SERVLET;
+    }
+
+    /**
+     * Retrieves the servlet instance for this additional servlet implementation.
+     * <p>
+     * The returned object's type must be compatible with the servlet interface class
+     * specified by the {@link AdditionalServletType} returned from {@link #getServletType()}.
+     * For example, if {@link #getServletType()} returns {@link AdditionalServletType#JAVAX_SERVLET},
+     * the returned object must implement {@code javax.servlet.Servlet}.
+     * </p>
      *
-     * @return the servlet holder
+     * @return the servlet instance implementing the appropriate servlet interface
      */
-    ServletHolder getServletHolder();
+    Object getServletInstance();
 
     @Override
     void close();
diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoader.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoader.java
@@ -24,7 +24,6 @@
 import lombok.extern.slf4j.Slf4j;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
 import org.apache.pulsar.common.nar.NarClassLoader;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 
 /**
  * An additional servlet with it's classloader.
@@ -60,11 +59,22 @@ public String getBasePath() {
     }
 
     @Override
-    public ServletHolder getServletHolder() {
+    public AdditionalServletType getServletType() {
         ClassLoader prevClassLoader = Thread.currentThread().getContextClassLoader();
         try {
             Thread.currentThread().setContextClassLoader(classLoader);
-            return servlet.getServletHolder();
+            return servlet.getServletType();
+        } finally {
+            Thread.currentThread().setContextClassLoader(prevClassLoader);
+        }
+    }
+
+    @Override
+    public Object getServletInstance() {
+        ClassLoader prevClassLoader = Thread.currentThread().getContextClassLoader();
+        try {
+            Thread.currentThread().setContextClassLoader(classLoader);
+            return servlet.getServletInstance();
         } finally {
             Thread.currentThread().setContextClassLoader(prevClassLoader);
         }
diff --git a/pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/MockAdditionalServlet.java b/pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/MockAdditionalServlet.java
@@ -19,7 +19,6 @@
 package org.apache.pulsar.broker.web.plugin.servlet;
 
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 
 public class MockAdditionalServlet implements AdditionalServlet {
 
@@ -34,7 +33,7 @@ public String getBasePath() {
     }
 
     @Override
-    public ServletHolder getServletHolder() {
+    public Object getServletInstance() {
         return null;
     }
 
diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
@@ -62,6 +62,7 @@
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
+import javax.servlet.Servlet;
 import javax.servlet.ServletException;
 import lombok.AccessLevel;
 import lombok.Getter;
@@ -1237,9 +1238,37 @@ private void addBrokerAdditionalServlets(WebService webService,
                 if (additionalServlet instanceof AdditionalServletWithPulsarService) {
                     ((AdditionalServletWithPulsarService) additionalServlet).setPulsarService(this);
                 }
-                webService.addServlet(servletWithClassLoader.getBasePath(), servletWithClassLoader.getServletHolder(),
-                        config.isAuthenticationEnabled(), attributeMap);
-                LOG.info("Broker add additional servlet basePath {} ", servletWithClassLoader.getBasePath());
+                switch (servletWithClassLoader.getServletType()) {
+                    case JAVAX_SERVLET -> {
+                        Object servletInstance = servletWithClassLoader.getServletInstance();
+                        if (!(servletInstance instanceof javax.servlet.Servlet)) {
+                            LOG.error("AdditionalServletWithClassLoader {} has invalid servlet instance type {} which "
+                                            + "doesn't match {}. Skipping.", servletWithClassLoader,
+                                    servletInstance.getClass().getName(), servletWithClassLoader.getServletType());
+                            try {
+                                servletWithClassLoader.close();
+                            } catch (Exception e) {
+                                LOG.error("Failed to close servlet {}.", servletWithClassLoader, e);
+                            }
+                            continue;
+                        }
+                        ServletHolder servletHolder =
+                                new ServletHolder((Servlet) servletInstance);
+                        webService.addServlet(servletWithClassLoader.getBasePath(), servletHolder,
+                                config.isAuthenticationEnabled(), attributeMap);
+                        LOG.info("Broker add additional servlet basePath {} ", servletWithClassLoader.getBasePath());
+                    }
+                    default -> {
+                        LOG.error("AdditionalServletWithClassLoader {} has unsupported servlet type {}. Skipping.",
+                                servletWithClassLoader, servletWithClassLoader.getServletType());
+                        try {
+                            servletWithClassLoader.close();
+                        } catch (Exception e) {
+                            LOG.error("Failed to close servlet {}.", servletWithClassLoader, e);
+                        }
+                        continue;
+                    }
+                }
             }
         }
     }
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerAdditionalServletTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerAdditionalServletTest.java
@@ -38,7 +38,6 @@
 import org.apache.pulsar.broker.web.plugin.servlet.AdditionalServlets;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
 import org.eclipse.jetty.ee8.nested.Request;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 import org.mockito.Mockito;
 import org.testng.Assert;
 import org.testng.annotations.AfterClass;
@@ -75,7 +74,7 @@ private void mockAdditionalServlet(PulsarService pulsar) {
 
         AdditionalServlet brokerAdditionalServlet = Mockito.mock(AdditionalServlet.class);
         Mockito.when(brokerAdditionalServlet.getBasePath()).thenReturn(BASE_PATH);
-        Mockito.when(brokerAdditionalServlet.getServletHolder()).thenReturn(new ServletHolder(servlet));
+        Mockito.when(brokerAdditionalServlet.getServletInstance()).thenReturn(servlet);
 
         AdditionalServletWithPulsarService brokerAdditionalServletWithPulsarService =
                 new AdditionalServletWithPulsarService() {
@@ -96,8 +95,8 @@ public String getBasePath() {
                     }
 
                     @Override
-                    public ServletHolder getServletHolder() {
-                        return new ServletHolder(new WithPulsarServiceServlet(pulsarService));
+                    public Object getServletInstance() {
+                        return new WithPulsarServiceServlet(pulsarService);
                     }
 
                     @Override
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoaderTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoaderTest.java
@@ -25,10 +25,10 @@
 import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNull;
+import javax.servlet.Servlet;
 import org.apache.pulsar.broker.ServiceConfiguration;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
 import org.apache.pulsar.common.nar.NarClassLoader;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 import org.testng.annotations.Test;
 
 
@@ -55,10 +55,9 @@ public void testWrapper() {
         // test getServlet
         assertEquals(wrapper.getServlet(), servlet);
         // test getServletHolder
-        ServletHolder servletHolder = new ServletHolder();
-        when(servlet.getServletHolder()).thenReturn(servletHolder);
-        assertEquals(wrapper.getServletHolder(), servletHolder);
-        verify(servlet, times(1)).getServletHolder();
+        Servlet servletInstance = mock(Servlet.class);
+        when(servlet.getServletInstance()).thenReturn(servletInstance);
+        verify(servlet, times(1)).getServletInstance();
     }
 
     @Test
@@ -77,7 +76,7 @@ public String getBasePath() {
             }
 
             @Override
-            public ServletHolder getServletHolder() {
+            public Object getServletInstance() {
                 assertEquals(Thread.currentThread().getContextClassLoader(), narLoader);
                 return null;
             }
@@ -101,7 +100,7 @@ public void close() {
         additionalServletWithClassLoader.loadConfig(conf);
         assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader);
         // test getServletHolder
-        assertNull(additionalServletWithClassLoader.getServletHolder());
+        assertNull(additionalServletWithClassLoader.getServletInstance());
         assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader);
         // test getServlet
         assertEquals(additionalServletWithClassLoader.getServlet(), servlet);
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/MockAdditionalServletWithClassLoader.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/MockAdditionalServletWithClassLoader.java
@@ -20,7 +20,6 @@
 
 import org.apache.pulsar.broker.PulsarService;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 
 public class MockAdditionalServletWithClassLoader implements AdditionalServletWithPulsarService{
     @Override
@@ -34,7 +33,7 @@ public String getBasePath() {
     }
 
     @Override
-    public ServletHolder getServletHolder() {
+    public Object getServletInstance() {
         return null;
     }
 
diff --git a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
@@ -38,6 +38,7 @@
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
+import javax.servlet.Servlet;
 import lombok.Getter;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.core.util.datetime.FixedDateFormat;
@@ -403,9 +404,37 @@ public static void addWebServerHandlers(WebServer server,
                     service.getProxyAdditionalServlets().getServlets().values();
             for (AdditionalServletWithClassLoader servletWithClassLoader : additionalServletCollection) {
                 servletWithClassLoader.loadConfig(config);
-                server.addServlet(servletWithClassLoader.getBasePath(), servletWithClassLoader.getServletHolder(),
-                        Collections.emptyList(), config.isAuthenticationEnabled());
-                log.info("proxy add additional servlet basePath {} ", servletWithClassLoader.getBasePath());
+                switch (servletWithClassLoader.getServletType()) {
+                    case JAVAX_SERVLET -> {
+                        Object servletInstance = servletWithClassLoader.getServletInstance();
+                        if (!(servletInstance instanceof javax.servlet.Servlet)) {
+                            log.error("AdditionalServletWithClassLoader {} has invalid servlet instance type {} which "
+                                            + "doesn't match {}. Skipping.", servletWithClassLoader,
+                                    servletInstance.getClass().getName(), servletWithClassLoader.getServletType());
+                            try {
+                                servletWithClassLoader.close();
+                            } catch (Exception e) {
+                                log.error("Failed to close servlet {}.", servletWithClassLoader, e);
+                            }
+                            continue;
+                        }
+                        ServletHolder additionalServletHolder =
+                                new ServletHolder((Servlet) servletInstance);
+                        server.addServlet(servletWithClassLoader.getBasePath(), additionalServletHolder,
+                                Collections.emptyList(), config.isAuthenticationEnabled());
+                        log.info("proxy add additional servlet basePath {} ", servletWithClassLoader.getBasePath());
+                    }
+                    default -> {
+                        log.error("AdditionalServletWithClassLoader {} has unsupported servlet type {}. Skipping.",
+                                servletWithClassLoader, servletWithClassLoader.getServletType());
+                        try {
+                            servletWithClassLoader.close();
+                        } catch (Exception e) {
+                            log.error("Failed to close servlet {}.", servletWithClassLoader, e);
+                        }
+                        continue;
+                    }
+                }
             }
         }
 
diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAdditionalServletTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAdditionalServletTest.java
@@ -50,7 +50,6 @@
 import org.apache.pulsar.common.util.ObjectMapperFactory;
 import org.apache.pulsar.metadata.impl.ZKMetadataStore;
 import org.eclipse.jetty.ee8.nested.Request;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 import org.mockito.Mockito;
 import org.testng.Assert;
 import org.testng.annotations.AfterClass;
@@ -177,7 +176,7 @@ public void destroy() {
 
         AdditionalServlet proxyAdditionalServlet = Mockito.mock(AdditionalServlet.class);
         Mockito.when(proxyAdditionalServlet.getBasePath()).thenReturn(BASE_PATH);
-        Mockito.when(proxyAdditionalServlet.getServletHolder()).thenReturn(new ServletHolder(servlet));
+        Mockito.when(proxyAdditionalServlet.getServletInstance()).thenReturn(servlet);
 
         AdditionalServlets proxyAdditionalServlets = Mockito.mock(AdditionalServlets.class);
         Map<String, AdditionalServletWithClassLoader> map = new HashMap<>();
diff --git a/tests/docker-images/java-test-plugins/src/main/java/org/apache/pulsar/tests/integration/plugins/RandomAdditionalServlet.java b/tests/docker-images/java-test-plugins/src/main/java/org/apache/pulsar/tests/integration/plugins/RandomAdditionalServlet.java
@@ -30,7 +30,6 @@
 import javax.servlet.http.HttpServletResponse;
 import org.apache.pulsar.broker.web.plugin.servlet.AdditionalServlet;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
-import org.eclipse.jetty.ee8.servlet.ServletHolder;
 
 public class RandomAdditionalServlet extends HttpServlet implements AdditionalServlet {
 
@@ -49,8 +48,8 @@ public String getBasePath() {
     }
 
     @Override
-    public ServletHolder getServletHolder() {
-        return new ServletHolder(this);
+    public Object getServletInstance() {
+        return this;
     }
 
     @Override

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,6 @@`
`19`	`19`	`package org.apache.pulsar.broker.web.plugin.servlet;`
`20`	`20`
`21`	`21`	`import org.apache.pulsar.common.configuration.PulsarConfiguration;`
`22`		`-import org.eclipse.jetty.ee8.servlet.ServletHolder;`
`23`	`22`
`24`	`23`	`public class MockAdditionalServlet implements AdditionalServlet {`
`25`	`24`
`@@ -34,7 +33,7 @@ public String getBasePath() {`
`34`	`33`	`}`
`35`	`34`
`36`	`35`	`@Override`
`37`		`- public ServletHolder getServletHolder() {`
	`36`	`+ public Object getServletInstance() {`
`38`	`37`	`return null;`
`39`	`38`	`}`
`40`	`39`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,6 @@`
`20`	`20`
`21`	`21`	`import org.apache.pulsar.broker.PulsarService;`
`22`	`22`	`import org.apache.pulsar.common.configuration.PulsarConfiguration;`
`23`		`-import org.eclipse.jetty.ee8.servlet.ServletHolder;`
`24`	`23`
`25`	`24`	`public class MockAdditionalServletWithClassLoader implements AdditionalServletWithPulsarService{`
`26`	`25`	`@Override`
`@@ -34,7 +33,7 @@ public String getBasePath() {`
`34`	`33`	`}`
`35`	`34`
`36`	`35`	`@Override`
`37`		`- public ServletHolder getServletHolder() {`
	`36`	`+ public Object getServletInstance() {`
`38`	`37`	`return null;`
`39`	`38`	`}`
`40`	`39`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,6 @@`
`30`	`30`	`import javax.servlet.http.HttpServletResponse;`
`31`	`31`	`import org.apache.pulsar.broker.web.plugin.servlet.AdditionalServlet;`
`32`	`32`	`import org.apache.pulsar.common.configuration.PulsarConfiguration;`
`33`		`-import org.eclipse.jetty.ee8.servlet.ServletHolder;`
`34`	`33`
`35`	`34`	`public class RandomAdditionalServlet extends HttpServlet implements AdditionalServlet {`
`36`	`35`
`@@ -49,8 +48,8 @@ public String getBasePath() {`
`49`	`48`	`}`
`50`	`49`
`51`	`50`	`@Override`
`52`		`- public ServletHolder getServletHolder() {`
`53`		`- return new ServletHolder(this);`
	`51`	`+ public Object getServletInstance() {`
	`52`	`+ return this;`
`54`	`53`	`}`
`55`	`54`
`56`	`55`	`@Override`