Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-10.0.x

Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java

@@ -68,6 +68,11 @@ public abstract class BaseHolder<T> extends AbstractLifeCycle implements Dumpabl
         return _lock.lock();
     }
 
+    boolean lockIsHeldByCurrentThread()
+    {
+        return _lock.isHeldByCurrentThread();
+    }
+
     /**
      * Do any setup necessary after starting
      *

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java

@@ -148,7 +148,6 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc
     private boolean _useFileMappedBuffer = false;
     private String _relativeResourceBase;
     private ServletHandler _servletHandler;
-    private ServletHolder _defaultHolder;
 
     public DefaultServlet(ResourceService resourceService)
     {
@@ -304,11 +303,6 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc
         _resourceService.setGzipEquivalentFileExtensions(gzipEquivalentFileExtensions);
 
         _servletHandler = _contextHandler.getChildHandlerByClass(ServletHandler.class);
-        for (ServletHolder h : _servletHandler.getServlets())
-        {
-            if (h.getServletInstance() == this)
-                _defaultHolder = h;
-        }
 
         if (LOG.isDebugEnabled())
             LOG.debug("resource base = {}", _resourceBase);
@@ -499,9 +493,9 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc
             return null;
 
         String welcomeServlet = null;
-        for (int i = 0; i < _welcomes.length; i++)
+        for (String s : _welcomes)
         {
-            String welcomeInContext = URIUtil.addPaths(pathInContext, _welcomes[i]);
+            String welcomeInContext = URIUtil.addPaths(pathInContext, s);
             Resource welcome = getResource(welcomeInContext);
             if (welcome != null && welcome.exists())
                 return welcomeInContext;
@@ -509,9 +503,7 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc
             if ((_welcomeServlets || _welcomeExactServlets) && welcomeServlet == null)
             {
                 ServletHandler.MappedServlet entry = _servletHandler.getMappedServlet(welcomeInContext);
-                @SuppressWarnings("ReferenceEquality")
+                if (entry != null && entry.getServletHolder().getServletInstance() != this &&
-                boolean isDefaultHolder = (entry.getServletHolder() != _defaultHolder);
-                if (entry != null && isDefaultHolder &&
                     (_welcomeServlets || (_welcomeExactServlets && entry.getPathSpec().getDeclaration().equals(welcomeInContext))))
                     welcomeServlet = welcomeInContext;
             }

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java

@@ -236,7 +236,8 @@ public class FilterHolder extends Holder<Filter>
     @Override
     public String toString()
     {
-        return String.format("%s@%x==%s,inst=%b,async=%b", getName(), hashCode(), getClassName(), _filter != null, isAsyncSupported());
+        return String.format("%s==%s@%x{inst=%b,async=%b,src=%s}",
+            getName(), getClassName(), hashCode(), _filter != null, isAsyncSupported(), getSource());
     }
 
     public FilterRegistration.Dynamic getRegistration()

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java

@@ -229,6 +229,7 @@ public class Invoker extends HttpServlet
         if (holder != null)
         {
             final Request baseRequest = Request.getBaseRequest(request);
+            holder.prepare(baseRequest, request, response);
             holder.handle(baseRequest,
                 new InvokedRequest(request, included, servlet, servletPath, pathInfo),
                 response);

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ListenerHolder.java

@@ -137,7 +137,7 @@ public class ListenerHolder extends BaseHolder<EventListener>
     @Override
     public String toString()
     {
-        return super.toString() + ": " + getClassName();
+        return String.format("%s@%x{src=%s}", getClassName(), hashCode(), getSource());
     }
 
     /**

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java

@@ -34,6 +34,7 @@ import java.util.Objects;
 import java.util.Set;
 import java.util.Stack;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
 import javax.servlet.GenericServlet;
 import javax.servlet.MultipartConfigElement;
 import javax.servlet.Servlet;
@@ -80,8 +81,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     private Map<String, String> _roleMap;
     private String _forcedPath;
     private String _runAsRole;
-    private RunAsToken _runAsToken;
-    private IdentityService _identityService;
     private ServletRegistration.Dynamic _registration;
     private JspContainer _jspContainer;
 
@@ -399,12 +398,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         checkInitOnStartup();
 
         _config = new Config();
-
-        try (AutoLock l = lock())
-        {
-            if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass()))
-                _servlet = new SingleThreadedWrapper();
-        }
     }
 
     @Override
@@ -452,13 +445,22 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
 
         Servlet servlet = (Servlet)o;
 
-        // need to use the unwrapped servlet because lifecycle callbacks such as
+        // call any predestroy callbacks
+        predestroyServlet(servlet);
+
+        // Call the servlet destroy
+        servlet.destroy();
+    }
+
+    private void predestroyServlet(Servlet servlet)
+    {
+        // TODO We should only predestroy instnaces that we created
+        // TODO But this breaks tests in jetty-9, so review behaviour in jetty-10
+
+        // Need to use the unwrapped servlet because lifecycle callbacks such as
         // postconstruct and predestroy are based off the classname and the wrapper
         // classes are unknown outside the ServletHolder
         getServletHandler().destroyServlet(unwrap(servlet));
-
-        // destroy the wrapped servlet, in case there is special behaviour
-        servlet.destroy();
     }
 
     /**
@@ -470,16 +472,20 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     public Servlet getServlet()
         throws ServletException
     {
-        try (AutoLock l = lock())
+        Servlet servlet = _servlet;
+        if (servlet == null)
         {
-            if (_servlet == null && isRunning())
+            try (AutoLock l = lock())
             {
-                if (getHeldClass() != null)
+                if (_servlet == null && isRunning())
-                    initServlet();
+                {
+                    if (getHeldClass() != null)
+                        initServlet();
+                }
+                servlet = _servlet;
             }
         }
-
+        return servlet;
-        return _servlet;
     }
 
     /**
@@ -533,7 +539,16 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     {
         try (AutoLock l = lock())
         {
-            _servlet = new UnavailableServlet(e, _servlet);
+            if (_servlet instanceof UnavailableServlet)
+            {
+                Throwable cause = ((UnavailableServlet)_servlet).getUnavailableException();
+                if (cause != e)
+                    cause.addSuppressed(e);
+            }
+            else
+            {
+                _servlet = new UnavailableServlet(e, _servlet);
+            }
             return _servlet;
         }
     }
@@ -562,33 +577,40 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     private void initServlet()
         throws ServletException
     {
-        try (AutoLock l = lock())
+        // must be called with lock held and _servlet==null
+        if (!lockIsHeldByCurrentThread())
+            throw new IllegalStateException("Lock not held");
+        if (_servlet != null)
+            throw new IllegalStateException("Servlet already initialised: " + _servlet);
+
+        Servlet servlet = null;
+        try
         {
-            if (_servlet == null)
+            servlet = getInstance();
-                _servlet = getInstance();
+            if (servlet == null)
-            if (_servlet == null)
+                servlet = newInstance();
-                _servlet = newInstance();
+            if (servlet instanceof javax.servlet.SingleThreadModel)
+            {
+                predestroyServlet(servlet);
+                servlet = new SingleThreadedWrapper();
+            }
+
             if (_config == null)
                 _config = new Config();
           
             //check run-as rolename and convert to token from IdentityService
-            if (_runAsRole == null)
+            if (_runAsRole != null)
             {
-                _identityService = null;
+                IdentityService identityService = getServletHandler().getIdentityService();
-                _runAsToken = null;
+                if (identityService != null)
-            }
-            else
-            {
-                _identityService = getServletHandler().getIdentityService();
-                if (_identityService != null)
                 {
-                    _runAsToken = _identityService.newRunAsToken(_runAsRole);
+                    RunAsToken runAsToken = identityService.newRunAsToken(_runAsRole);
-                    _servlet = new RunAs(_servlet, _identityService, _runAsToken);
+                    servlet = new RunAs(servlet, identityService, runAsToken);
                 }
             }
 
             if (!isAsyncSupported())
-                _servlet = new NotAsync(_servlet);
+                servlet = new NotAsync(servlet);
 
             // Handle configuring servlets that implement org.apache.jasper.servlet.JspServlet
             if (isJspServlet())
@@ -599,28 +621,30 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
             else if (_forcedPath != null)
                 detectJspContainer();
 
-            _servlet = wrap(_servlet, WrapFunction.class, WrapFunction::wrapServlet);
+            servlet = wrap(servlet, WrapFunction.class, WrapFunction::wrapServlet);
 
             if (LOG.isDebugEnabled())
                 LOG.debug("Servlet.init {} for {}", _servlet, getName());
-            _servlet.init(_config);
+            try
-        }
+            {
-        catch (UnavailableException e)
+                servlet.init(_config);
-        {
+                _servlet = servlet;
-            makeUnavailable(e);
+            }
-            if (getServletHandler().isStartWithUnavailable())
+            catch (UnavailableException e)
-                LOG.warn("{} is marked as Unavailable", this, e);
+            {
-            else
+                _servlet = new UnavailableServlet(e, servlet);
-                throw e;
+            }
         }
         catch (ServletException e)
         {
             makeUnavailable(e.getCause() == null ? e : e.getCause());
+            predestroyServlet(servlet);
             throw e;
         }
         catch (Exception e)
         {
             makeUnavailable(e);
+            predestroyServlet(servlet);
             throw new ServletException(this.toString(), e);
         }
     }
@@ -655,8 +679,8 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         }
 
         scratch = new File(getInitParameter("scratchdir"));
-        if (!scratch.exists())
+        if (!scratch.exists() && !scratch.mkdir())
-            scratch.mkdir();
+            throw new IllegalStateException("Could not create JSP scratch directory");
     }
 
     @Override
@@ -700,10 +724,16 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     protected void prepare(Request baseRequest, ServletRequest request, ServletResponse response)
         throws ServletException, UnavailableException
     {
+        // Ensure the servlet is initialized prior to any filters being invoked
         getServlet();
-        MultipartConfigElement mpce = ((Registration)getRegistration()).getMultipartConfig();
+
-        if (mpce != null)
+        // Check for multipart config
-            baseRequest.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce);
+        if (_registration != null)
+        {
+            MultipartConfigElement mpce = ((Registration)_registration).getMultipartConfig();
+            if (mpce != null)
+                baseRequest.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce);
+        }
     }
 
     /**
@@ -725,7 +755,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     {
         try
         {
-            Servlet servlet = getServlet();
+            Servlet servlet = getServletInstance();
             if (servlet == null)
                 throw new UnavailableException("Servlet Not Initialized");
             servlet.service(request, response);
@@ -1160,72 +1190,69 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     @Override
     public String toString()
     {
-        return String.format("%s@%x==%s,jsp=%s,order=%d,inst=%b,async=%b", getName(), hashCode(), getClassName(), _forcedPath, _initOrder, _servlet != null, isAsyncSupported());
+        return String.format("%s==%s@%x{jsp=%s,order=%d,inst=%b,async=%b,src=%s}",
+            getName(), getClassName(), hashCode(),
+            _forcedPath, _initOrder, _servlet != null, isAsyncSupported(), getSource());
     }
 
-    private class UnavailableServlet extends GenericServlet
+    private class UnavailableServlet extends Wrapper
     {
         final UnavailableException _unavailableException;
-        final Servlet _servlet;
+        final AtomicLong _unavailableStart;
-        final long _available;
 
         public UnavailableServlet(UnavailableException unavailableException, Servlet servlet)
         {
+            super(servlet != null ? servlet : new GenericServlet()
+            {
+                @Override
+                public void service(ServletRequest req, ServletResponse res) throws IOException
+                {
+                    ((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND);
+                }
+            });
             _unavailableException = unavailableException;
 
             if (unavailableException.isPermanent())
-            {
+                _unavailableStart = null;
-                _servlet = null;
-                _available = -1;
-                if (servlet != null)
-                {
-                    try
-                    {
-                        destroyInstance(servlet);
-                    }
-                    catch (Throwable th)
-                    {
-                        if (th != unavailableException)
-                            unavailableException.addSuppressed(th);
-                    }
-                }
-            }
             else
             {
-                _servlet = servlet;
+                long start = System.nanoTime();
-                _available = System.nanoTime() + TimeUnit.SECONDS.toNanos(unavailableException.getUnavailableSeconds());
+                while (start == 0)
+                    start = System.nanoTime();
+                _unavailableStart = new AtomicLong(start);
             }
         }
 
         @Override
         public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
         {
-            if (_available == -1)
+            if (LOG.isDebugEnabled())
+               LOG.debug("Unavailable {}", req, _unavailableException);
+            if (_unavailableStart == null)
+            {
                 ((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND);
-            else if (System.nanoTime() < _available)
+            }
-                ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
             else
             {
-                try (AutoLock l = lock())
+                long start = _unavailableStart.get();
-                {
-                    ServletHolder.this._servlet = this._servlet;
-                    _servlet.service(req, res);
-                }
-            }
-        }
 
-        @Override
+                if (start == 0 || System.nanoTime() - start < TimeUnit.SECONDS.toNanos(_unavailableException.getUnavailableSeconds()))
-        public void destroy()
-        {
-            if (_servlet != null)
-            {
-                try
                 {
-                    destroyInstance(_servlet);
+                    ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
                 }
-                catch (Throwable th)
+                else if (_unavailableStart.compareAndSet(start, 0))
                 {
-                    LOG.warn("Unable to destroy {}", _servlet, th);
+                    try (AutoLock l = lock())
+                    {
+                        _servlet = getWrapped();
+                    }
+                    Request baseRequest = Request.getBaseRequest(req);
+                    ServletHolder.this.prepare(baseRequest, req, res);
+                    ServletHolder.this.handle(baseRequest, req, res);
+                }
+                else
+                {
+                    ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
                 }
             }
         }
@@ -1257,53 +1284,53 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
 
     public static class Wrapper implements Servlet, Wrapped<Servlet>
     {
-        private final Servlet _servlet;
+        private final Servlet _wrappedServlet;
 
         public Wrapper(Servlet servlet)
         {
-            _servlet = Objects.requireNonNull(servlet, "Servlet cannot be null");
+            _wrappedServlet = Objects.requireNonNull(servlet, "Servlet cannot be null");
         }
 
         @Override
         public Servlet getWrapped()
         {
-            return _servlet;
+            return _wrappedServlet;
         }
 
         @Override
         public void init(ServletConfig config) throws ServletException
         {
-            _servlet.init(config);
+            _wrappedServlet.init(config);
         }
 
         @Override
         public ServletConfig getServletConfig()
         {
-            return _servlet.getServletConfig();
+            return _wrappedServlet.getServletConfig();
         }
 
         @Override
         public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
         {
-            _servlet.service(req, res);
+            _wrappedServlet.service(req, res);
         }
 
         @Override
         public String getServletInfo()
         {
-            return _servlet.getServletInfo();
+            return _wrappedServlet.getServletInfo();
         }
 
         @Override
         public void destroy()
         {
-            _servlet.destroy();
+            _wrappedServlet.destroy();
         }
 
         @Override
         public String toString()
         {
-            return String.format("%s:%s", this.getClass().getSimpleName(), _servlet.toString());
+            return String.format("%s:%s", this.getClass().getSimpleName(), _wrappedServlet.toString());
         }
     }
 

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java

@@ -459,6 +459,7 @@ public class ErrorPageTest
                 __destroyed = new AtomicBoolean(false);
                 String response = _connector.getResponse("GET /unavailable/info HTTP/1.0\r\n\r\n");
                 assertThat(response, Matchers.containsString("HTTP/1.1 404 "));
+                _server.stop();
                 assertTrue(__destroyed.get());
             }
         }

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java

@@ -60,6 +60,10 @@ public class ServletLifeCycleTest
 
         context.getObjectFactory().addDecorator(new TestDecorator());
 
+        // TODO review this test in jetty-10.  Instances that are created externally and passed in should not be
+        // TODO decorated by the object factory unless: a) there is an explicit call to ServletContext.createXxx;
+        // TODO ; and b) the Servlet dyanmic API is used to register them.
+
         ServletHandler sh = context.getServletHandler();
         sh.addListener(new ListenerHolder(TestListener.class)); //added directly to ServletHandler
         context.addEventListener(context.getServletContext().createListener(TestListener2.class));//create,decorate and add listener to context - no holder!

jetty-util/src/main/java/org/eclipse/jetty/util/component/AbstractLifeCycle.java

@@ -23,6 +23,7 @@ import java.util.EventListener;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
 
+import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.Uptime;
 import org.eclipse.jetty.util.annotation.ManagedAttribute;
 import org.eclipse.jetty.util.annotation.ManagedObject;
@@ -334,13 +335,9 @@ public abstract class AbstractLifeCycle implements LifeCycle
     @Override
     public String toString()
     {
-        Class<?> clazz = getClass();
+        String name = getClass().getSimpleName();
-        String name = clazz.getSimpleName();
+        if (StringUtil.isBlank(name) && getClass().getSuperclass() != null)
-        if ((name == null || name.length() == 0) && clazz.getSuperclass() != null)
+            name = getClass().getSuperclass().getSimpleName();
-        {
-            clazz = clazz.getSuperclass();
-            name = clazz.getSimpleName();
-        }
         return String.format("%s@%x{%s}", name, hashCode(), getState());
     }