From 721a05eb9d71d6ba6ee4ff57555a8a4efcef50ed Mon Sep 17 00:00:00 2001
From: Greg Wilkins <gregw@webtide.com>
Date: Wed, 7 Oct 2020 23:22:55 +0200
Subject: [PATCH] Fixes #5378 Setting Holders during STARTING (#5397)

* Fixes #5378 Setting Holders during STARTING

Holders are now started/initialized if needed by a new utility method

* Fixes #5378 Setting Holders during STARTING

Holders are now started/initialized if needed by a new utility method
---
 .../eclipse/jetty/servlet/ListenerHolder.java |  42 ++---
 .../eclipse/jetty/servlet/ServletHandler.java |  57 +++---
 .../servlet/ServletContextHandlerTest.java    | 171 ++++++++++++++++--
 3 files changed, 203 insertions(+), 67 deletions(-)

diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ListenerHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ListenerHolder.java
index ef10588e5d3c..42d4c0549a82 100644
--- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ListenerHolder.java
+++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ListenerHolder.java
@@ -19,7 +19,6 @@
 package org.eclipse.jetty.servlet;
 
 import java.util.EventListener;
-import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
 
 import org.eclipse.jetty.server.handler.ContextHandler;
@@ -78,33 +77,30 @@ public void doStart() throws Exception
             throw new IllegalStateException(msg);
         }
 
-        ContextHandler contextHandler = ContextHandler.getCurrentContext().getContextHandler();
-        if (contextHandler != null)
+        ContextHandler contextHandler = null;
+        if (getServletHandler() != null)
+            contextHandler = getServletHandler().getServletContextHandler();
+        if (contextHandler == null && ContextHandler.getCurrentContext() != null)
+            contextHandler = ContextHandler.getCurrentContext().getContextHandler();
+        if (contextHandler == null)
+            throw new IllegalStateException("No Context");
+
+        _listener = getInstance();
+        if (_listener == null)
         {
-            _listener = getInstance();
-            if (_listener == null)
+            //create an instance of the listener and decorate it
+            try
             {
-                //create an instance of the listener and decorate it
-                try
-                {
-                    ServletContext context = contextHandler.getServletContext();
-                    _listener = (context != null)
-                        ? context.createListener(getHeldClass())
-                        : getHeldClass().getDeclaredConstructor().newInstance();
-                }
-                catch (ServletException ex)
-                {
-                    Throwable cause = ex.getRootCause();
-                    if (cause instanceof InstantiationException)
-                        throw (InstantiationException)cause;
-                    if (cause instanceof IllegalAccessException)
-                        throw (IllegalAccessException)cause;
-                    throw ex;
-                }
+                _listener = contextHandler.getServletContext().createListener(getHeldClass());
             }
+            catch (ServletException ex)
+            {
+                throw ex;
+            }
+
             _listener = wrap(_listener, WrapFunction.class, WrapFunction::wrapEventListener);
-            contextHandler.addEventListener(_listener);
         }
+        contextHandler.addEventListener(_listener);
     }
 
     @Override
diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
index 32d80bb99084..ba41caf32858 100644
--- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
+++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
@@ -773,6 +773,29 @@ public boolean isInitialized()
         return _initialized;
     }
 
+    protected void initializeHolders(BaseHolder<?>[] holders)
+    {
+        for (BaseHolder<?> holder : holders)
+        {
+            holder.setServletHandler(this);
+            if (isInitialized())
+            {
+                try
+                {
+                    if (!holder.isStarted())
+                    {
+                        holder.start();
+                        holder.initialize();
+                    }
+                }
+                catch (Exception e)
+                {
+                    throw new RuntimeException(e);
+                }
+            }
+        }
+    }
+
     /**
      * @return whether the filter chains are cached.
      */
@@ -800,10 +823,7 @@ public ListenerHolder[] getListeners()
     public void setListeners(ListenerHolder[] listeners)
     {
         if (listeners != null)
-            for (ListenerHolder holder : listeners)
-            {
-                holder.setServletHandler(this);
-            }
+            initializeHolders(listeners);
         updateBeans(_listeners,listeners);
         _listeners = listeners;
     }
@@ -865,9 +885,6 @@ public void addServletWithMapping(ServletHolder servlet, String pathSpec)
     {
         Objects.requireNonNull(servlet);
         ServletHolder[] holders = getServlets();
-        if (holders != null)
-            holders = holders.clone();
-
         try
         {
             synchronized (this)
@@ -979,8 +996,6 @@ public void addFilterWithMapping(FilterHolder holder, String pathSpec, EnumSet<D
     {
         Objects.requireNonNull(holder);
         FilterHolder[] holders = getFilters();
-        if (holders != null)
-            holders = holders.clone();
 
         try
         {
@@ -1435,16 +1450,6 @@ else if (isAllowDuplicateMappings())
             LOG.debug("servletPathMap=" + _servletPathMap);
             LOG.debug("servletNameMap=" + _servletNameMap);
         }
-
-        try
-        {
-            if (_contextHandler != null && _contextHandler.isStarted() || _contextHandler == null && isStarted())
-                initialize();
-        }
-        catch (Exception e)
-        {
-            throw new RuntimeException(e);
-        }
     }
 
     protected void notFound(Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
@@ -1496,7 +1501,7 @@ public void setFilterMappings(FilterMapping[] filterMappings)
     {
         updateBeans(_filterMappings,filterMappings);
         _filterMappings = filterMappings;
-        if (isStarted())
+        if (isRunning())
             updateMappings();
         invalidateChainsCache();
     }
@@ -1504,10 +1509,7 @@ public void setFilterMappings(FilterMapping[] filterMappings)
     public synchronized void setFilters(FilterHolder[] holders)
     {
         if (holders != null)
-            for (FilterHolder holder : holders)
-            {
-                holder.setServletHandler(this);
-            }
+            initializeHolders(holders);
         updateBeans(_filters,holders);
         _filters = holders;
         updateNameMappings();
@@ -1521,7 +1523,7 @@ public void setServletMappings(ServletMapping[] servletMappings)
     {
         updateBeans(_servletMappings,servletMappings);
         _servletMappings = servletMappings;
-        if (isStarted())
+        if (isRunning())
             updateMappings();
         invalidateChainsCache();
     }
@@ -1534,10 +1536,7 @@ public void setServletMappings(ServletMapping[] servletMappings)
     public synchronized void setServlets(ServletHolder[] holders)
     {
         if (holders != null)
-            for (ServletHolder holder : holders)
-            {
-                holder.setServletHandler(this);
-            }
+            initializeHolders(holders);
         updateBeans(_servlets,holders);
         _servlets = holders;
         updateNameMappings();
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletContextHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletContextHandlerTest.java
index fe076310ae36..3119dec21a9d 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletContextHandlerTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletContextHandlerTest.java
@@ -1644,38 +1644,179 @@ else if ("delete".equalsIgnoreCase(action))
         }
     }
 
+    public static class TestPListener implements  ServletRequestListener
+    {
+        @Override
+        public void requestInitialized(ServletRequestEvent sre)
+        {
+            ServletRequest request = sre.getServletRequest();
+            Integer count = (Integer)request.getAttribute("testRequestListener");
+            request.setAttribute("testRequestListener", count == null ? 1 : count + 1);
+        }
+
+        @Override
+        public void requestDestroyed(ServletRequestEvent sre)
+        {
+        }
+    }
+
     @Test
-    public void testProgrammaticFilterServlet() throws Exception
+    public void testProgrammaticListener() throws Exception
     {
         ServletContextHandler context = new ServletContextHandler();
         ServletHandler handler = new ServletHandler();
         _server.setHandler(context);
         context.setHandler(handler);
-        handler.addServletWithMapping(new ServletHolder(new TestServlet()), "/");
 
+        // Add a servlet to report number of listeners
+        handler.addServletWithMapping(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+            {
+                resp.getOutputStream().print("Listeners=" + req.getAttribute("testRequestListener"));
+            }
+        }), "/");
+
+        // Add a listener in STOPPED, STARTING and STARTED states
+        handler.addListener(new ListenerHolder(TestPListener.class));
+        handler.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            public void init() throws ServletException
+            {
+                handler.addListener(new ListenerHolder(TestPListener.class));
+            }
+        })
+        {
+            {
+                setInitOrder(1);
+            }
+        });
         _server.start();
+        handler.addListener(new ListenerHolder(TestPListener.class));
 
+        String request =
+            "GET /test HTTP/1.0\n" +
+                "Host: localhost\n" +
+                "\n";
+        String response = _connector.getResponse(request);
+        assertThat(response, containsString("200 OK"));
+        assertThat(response, containsString("Listeners=3"));
+    }
+
+    public static class TestPFilter implements Filter
+    {
+        @Override
+        public void init(FilterConfig filterConfig) throws ServletException
+        {
+        }
+
+        @Override
+        public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
+        {
+            Integer count = (Integer)request.getAttribute("testFilter");
+            request.setAttribute("testFilter", count == null ? 1 : count + 1);
+            chain.doFilter(request, response);
+        }
+
+        @Override
+        public void destroy()
+        {
+        }
+    }
+
+    @Test
+    public void testProgrammaticFilters() throws Exception
+    {
+        ServletContextHandler context = new ServletContextHandler();
+        ServletHandler handler = new ServletHandler();
+        _server.setHandler(context);
+        context.setHandler(handler);
+
+        // Add a servlet to report number of filters
+        handler.addServletWithMapping(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+            {
+                resp.getOutputStream().print("Filters=" + req.getAttribute("testFilter"));
+            }
+        }), "/");
+
+        // Add a filter in STOPPED, STARTING and STARTED states
+        handler.addFilterWithMapping(new FilterHolder(TestPFilter.class), "/*", EnumSet.of(DispatcherType.REQUEST));
+        handler.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            public void init() throws ServletException
+            {
+                handler.addFilterWithMapping(new FilterHolder(TestPFilter.class), "/*", EnumSet.of(DispatcherType.REQUEST));
+            }
+        })
+        {
+            {
+                setInitOrder(1);
+            }
+        });
+        _server.start();
+        handler.addFilterWithMapping(new FilterHolder(TestPFilter.class), "/*", EnumSet.of(DispatcherType.REQUEST));
 
         String request =
-            "GET /hello HTTP/1.0\n" +
-            "Host: localhost\n" +
-            "\n";
+            "GET /test HTTP/1.0\n" +
+                "Host: localhost\n" +
+                "\n";
         String response = _connector.getResponse(request);
         assertThat(response, containsString("200 OK"));
-        assertThat(response, containsString("Test"));
+        assertThat(response, containsString("Filters=3"));
+    }
+
+    public static class TestPServlet extends HttpServlet
+    {
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+        {
+            resp.getOutputStream().println(req.getRequestURI());
+        }
+    }
 
-        handler.addFilterWithMapping(new FilterHolder(new MyFilter()), "/*", EnumSet.of(DispatcherType.REQUEST));
-        handler.addServletWithMapping(new ServletHolder(new HelloServlet()), "/hello/*");
+    @Test
+    public void testProgrammaticServlets() throws Exception
+    {
+        ServletContextHandler context = new ServletContextHandler();
+        ServletHandler handler = new ServletHandler();
+        _server.setHandler(context);
+        context.setHandler(handler);
 
-        _server.dumpStdErr();
+        // Add a filter in STOPPED, STARTING and STARTED states
+        handler.addServletWithMapping(new ServletHolder(TestPServlet.class), "/one");
+        handler.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            public void init() throws ServletException
+            {
+                handler.addServletWithMapping(new ServletHolder(TestPServlet.class), "/two");
+            }
+        })
+        {
+            {
+                setInitOrder(1);
+            }
+        });
+        _server.start();
+        handler.addServletWithMapping(new ServletHolder(TestPServlet.class), "/three");
 
-        request =
-            "GET /hello HTTP/1.0\n" +
-            "Host: localhost\n" +
-            "\n";
+        String request = "GET /one HTTP/1.0\n" + "Host: localhost\n" + "\n";
+        String response = _connector.getResponse(request);
+        assertThat(response, containsString("200 OK"));
+        assertThat(response, containsString("/one"));
+        request = "GET /two HTTP/1.0\n" + "Host: localhost\n" + "\n";
         response = _connector.getResponse(request);
         assertThat(response, containsString("200 OK"));
-        assertThat(response, containsString("filter: filter"));
-        assertThat(response, containsString("Hello World"));
+        assertThat(response, containsString("/two"));
+        request = "GET /three HTTP/1.0\n" + "Host: localhost\n" + "\n";
+        response = _connector.getResponse(request);
+        assertThat(response, containsString("200 OK"));
+        assertThat(response, containsString("/three"));
     }
 }
\ No newline at end of file