Merge branch 'jetty-9.4.x' into jetty-10.0.x

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

# Conflicts:
#	jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java
#	jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java

@@ -889,9 +889,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
 
     protected void stopContext() throws Exception
     {
-        // stop all the handler hierarchy
-        super.doStop();
-
         // Call the context listeners
         ServletContextEvent event = new ServletContextEvent(_scontext);
         Collections.reverse(_destroySerletContextListeners);
@@ -907,6 +904,17 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
                 ex.add(x);
             }
         }
+
+        // stop all the handler hierarchy
+        try
+        {
+            super.doStop();
+        }
+        catch (Exception x)
+        {
+            ex.add(x);
+        }
+
         ex.ifExceptionThrow();
     }
 

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

@@ -43,11 +43,11 @@ public abstract class BaseHolder<T> extends AbstractLifeCycle implements Dumpabl
 {
     private static final Logger LOG = Log.getLogger(BaseHolder.class);
 
-    protected final Source _source;
+    private final Source _source;
-    protected transient Class<? extends T> _class;
+    private Class<? extends T> _class;
-    protected String _className;
+    private String _className;
-    protected boolean _extInstance;
+    private T _instance;
-    protected ServletHandler _servletHandler;
+    private ServletHandler _servletHandler;
 
     protected BaseHolder(Source source)
     {
@@ -101,7 +101,7 @@ public abstract class BaseHolder<T> extends AbstractLifeCycle implements Dumpabl
     public void doStop()
         throws Exception
     {
-        if (!_extInstance)
+        if (_instance == null)
             _class = null;
     }
 
@@ -163,12 +163,26 @@ public abstract class BaseHolder<T> extends AbstractLifeCycle implements Dumpabl
         }
     }
 
+    protected synchronized void setInstance(T instance)
+    {
+        _instance = instance;
+        if (instance == null)
+            setHeldClass(null);
+        else
+            setHeldClass((Class<T>)instance.getClass());
+    }
+
+    protected synchronized T getInstance()
+    {
+        return _instance;
+    }
+
     /**
      * @return True if this holder was created for a specific instance.
      */
-    public boolean isInstance()
+    public synchronized boolean isInstance()
     {
-        return _extInstance;
+        return _instance != null;
     }
 
     @Override

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

@@ -91,11 +91,10 @@ public class FilterHolder extends Holder<Filter>
     {
         super.doStart();
 
-        if (!javax.servlet.Filter.class
+        if (!javax.servlet.Filter.class.isAssignableFrom(getHeldClass()))
-            .isAssignableFrom(_class))
         {
-            String msg = _class + " is not a javax.servlet.Filter";
+            String msg = getHeldClass() + " is not a javax.servlet.Filter";
-            super.stop();
+            doStop();
             throw new IllegalStateException(msg);
         }
     }
@@ -103,15 +102,18 @@ public class FilterHolder extends Holder<Filter>
     @Override
     public void initialize() throws Exception
     {
-        if (!_initialized)
+        synchronized (this)
         {
-            super.initialize();
+            if (_filter != null)
+                return;
 
+            super.initialize();
+            _filter = getInstance();
             if (_filter == null)
             {
                 try
                 {
-                    ServletContext context = _servletHandler.getServletContext();
+                    ServletContext context = getServletHandler().getServletContext();
                     _filter = (context instanceof ServletContextHandler.Context)
                         ? context.createFilter(getHeldClass())
                         : getHeldClass().getDeclaredConstructor().newInstance();
@@ -126,37 +128,30 @@ public class FilterHolder extends Holder<Filter>
                     throw ex;
                 }
             }
-
             _config = new Config();
             if (LOG.isDebugEnabled())
                 LOG.debug("Filter.init {}", _filter);
             _filter.init(_config);
         }
-
-        _initialized = true;
     }
 
     @Override
     public void doStop()
         throws Exception
     {
+        super.doStop();
+        _config = null;
         if (_filter != null)
         {
             try
             {
                 destroyInstance(_filter);
             }
-            catch (Exception e)
+            finally
             {
-                LOG.warn(e);
+                _filter = null;
             }
         }
-        if (!_extInstance)
-            _filter = null;
-
-        _config = null;
-        _initialized = false;
-        super.doStop();
     }
 
     @Override
@@ -172,11 +167,7 @@ public class FilterHolder extends Holder<Filter>
 
     public synchronized void setFilter(Filter filter)
     {
-        _filter = filter;
+        setInstance(filter);
-        _extInstance = true;
-        setHeldClass(filter.getClass());
-        if (getName() == null)
-            setName(filter.getClass().getName());
     }
 
     public Filter getFilter()
@@ -187,19 +178,19 @@ public class FilterHolder extends Holder<Filter>
     @Override
     public void dump(Appendable out, String indent) throws IOException
     {
-        if (_initParams.isEmpty())
+        if (getInitParameters().isEmpty())
             Dumpable.dumpObjects(out, indent, this,
                 _filter == null ? getHeldClass() : _filter);
         else
             Dumpable.dumpObjects(out, indent, this,
                 _filter == null ? getHeldClass() : _filter,
-                new DumpableCollection("initParams", _initParams.entrySet()));
+                new DumpableCollection("initParams", getInitParameters().entrySet()));
     }
 
     @Override
     public String toString()
     {
-        return String.format("%s@%x==%s,inst=%b,async=%b", _name, hashCode(), _className, _filter != null, isAsyncSupported());
+        return String.format("%s@%x==%s,inst=%b,async=%b", getName(), hashCode(), getClassName(), _filter != null, isAsyncSupported());
     }
 
     public FilterRegistration.Dynamic getRegistration()
@@ -220,9 +211,9 @@ public class FilterHolder extends Holder<Filter>
             mapping.setServletNames(servletNames);
             mapping.setDispatcherTypes(dispatcherTypes);
             if (isMatchAfter)
-                _servletHandler.addFilterMapping(mapping);
+                getServletHandler().addFilterMapping(mapping);
             else
-                _servletHandler.prependFilterMapping(mapping);
+                getServletHandler().prependFilterMapping(mapping);
         }
 
         @Override
@@ -234,15 +225,15 @@ public class FilterHolder extends Holder<Filter>
             mapping.setPathSpecs(urlPatterns);
             mapping.setDispatcherTypes(dispatcherTypes);
             if (isMatchAfter)
-                _servletHandler.addFilterMapping(mapping);
+                getServletHandler().addFilterMapping(mapping);
             else
-                _servletHandler.prependFilterMapping(mapping);
+                getServletHandler().prependFilterMapping(mapping);
         }
 
         @Override
         public Collection<String> getServletNameMappings()
         {
-            FilterMapping[] mappings = _servletHandler.getFilterMappings();
+            FilterMapping[] mappings = getServletHandler().getFilterMappings();
             List<String> names = new ArrayList<String>();
             for (FilterMapping mapping : mappings)
             {
@@ -258,7 +249,7 @@ public class FilterHolder extends Holder<Filter>
         @Override
         public Collection<String> getUrlPatternMappings()
         {
-            FilterMapping[] mappings = _servletHandler.getFilterMappings();
+            FilterMapping[] mappings = getServletHandler().getFilterMappings();
             List<String> patterns = new ArrayList<String>();
             for (FilterMapping mapping : mappings)
             {
@@ -277,7 +268,7 @@ public class FilterHolder extends Holder<Filter>
         @Override
         public String getFilterName()
         {
-            return _name;
+            return getName();
         }
     }
 }

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

@@ -45,16 +45,15 @@ public abstract class Holder<T> extends BaseHolder<T>
 {
     private static final Logger LOG = Log.getLogger(Holder.class);
 
-    protected final Map<String, String> _initParams = new HashMap<String, String>(3);
+    private final Map<String, String> _initParams = new HashMap<String, String>(3);
-    protected String _displayName;
+    private String _displayName;
-    protected boolean _asyncSupported;
+    private boolean _asyncSupported;
-    protected String _name;
+    private String _name;
-    protected boolean _initialized = false;
 
     protected Holder(Source source)
     {
         super(source);
-        switch (_source.getOrigin())
+        switch (getSource().getOrigin())
         {
             case JAVAX_API:
             case DESCRIPTOR:
@@ -98,6 +97,14 @@ public abstract class Holder<T> extends BaseHolder<T>
         return _name;
     }
 
+    @Override
+    protected synchronized void setInstance(T instance)
+    {
+        super.setInstance(instance);
+        if (getName() == null)
+            setName(String.format("%s@%x", instance.getClass().getName(), instance.hashCode()));
+    }
+
     public void destroyInstance(Object instance)
         throws Exception
     {
@@ -175,7 +182,7 @@ public abstract class Holder<T> extends BaseHolder<T>
     @Override
     public String toString()
     {
-        return String.format("%s@%x==%s", _name, hashCode(), _className);
+        return String.format("%s@%x==%s", _name, hashCode(), getClassName());
     }
 
     protected class HolderConfig
@@ -183,7 +190,7 @@ public abstract class Holder<T> extends BaseHolder<T>
 
         public ServletContext getServletContext()
         {
-            return _servletHandler.getServletContext();
+            return getServletHandler().getServletContext();
         }
 
         public String getInitParameter(String param)

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

@@ -64,52 +64,66 @@ public class ListenerHolder extends BaseHolder<EventListener>
      */
     public void setListener(EventListener listener)
     {
-        _listener = listener;
+        setInstance(listener);
-        _extInstance = true;
-        setHeldClass(_listener.getClass());
     }
 
     @Override
     public void doStart() throws Exception
     {
         super.doStart();
-        if (!java.util.EventListener.class.isAssignableFrom(_class))
+        if (!java.util.EventListener.class.isAssignableFrom(getHeldClass()))
         {
-            String msg = _class + " is not a java.util.EventListener";
+            String msg = getHeldClass() + " is not a java.util.EventListener";
             super.stop();
             throw new IllegalStateException(msg);
         }
-        
+
         ContextHandler contextHandler = ContextHandler.getCurrentContext().getContextHandler();
-        if (_listener == null)
+        if (contextHandler != null)
         {
-            //create an instance of the listener and decorate it
+            _listener = getInstance();
-            try
+            if (_listener == null)
             {
-                ServletContext scontext = contextHandler.getServletContext();
+                //create an instance of the listener and decorate it
-                _listener = (scontext instanceof ServletContextHandler.Context)
+                try
-                    ? scontext.createListener(getHeldClass())
+                {
-                    : getHeldClass().getDeclaredConstructor().newInstance();
+                    ServletContext scontext = contextHandler.getServletContext();
-            }
+                    _listener = (scontext instanceof ServletContextHandler.Context)
-            catch (ServletException ex)
+                        ? scontext.createListener(getHeldClass())
-            {
+                        : getHeldClass().getDeclaredConstructor().newInstance();
-                Throwable cause = ex.getRootCause();
+                }
-                if (cause instanceof InstantiationException)
+                catch (ServletException ex)
-                    throw (InstantiationException)cause;
+                {
-                if (cause instanceof IllegalAccessException)
+                    Throwable cause = ex.getRootCause();
-                    throw (IllegalAccessException)cause;
+                    if (cause instanceof InstantiationException)
-                throw ex;
+                        throw (InstantiationException)cause;
+                    if (cause instanceof IllegalAccessException)
+                        throw (IllegalAccessException)cause;
+                    throw ex;
+                }
             }
+            contextHandler.addEventListener(_listener);
         }
-        contextHandler.addEventListener(_listener);
     }
 
     @Override
     public void doStop() throws Exception
     {
         super.doStop();
-        if (!_extInstance)
+        if (_listener != null)
-            _listener = null;
+        {
+            try
+            {
+                ContextHandler contextHandler = ContextHandler.getCurrentContext().getContextHandler();
+                if (contextHandler != null)
+                    contextHandler.removeEventListener(_listener);
+                getServletHandler().destroyListener(_listener);
+            }
+            finally
+            {
+                _listener = null;
+            }
+        }
     }
 
     @Override

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

@@ -121,8 +121,6 @@ public class ServletContextHandler extends ContextHandler
 
     public interface ServletContainerInitializerCaller extends LifeCycle {}
 
-    ;
-
     protected final DecoratedObjectFactory _objFactory;
     protected Class<? extends SecurityHandler> _defaultSecurityHandlerClass = org.eclipse.jetty.security.ConstraintSecurityHandler.class;
     protected SessionHandler _sessionHandler;
@@ -731,6 +729,11 @@ public class ServletContextHandler extends ContextHandler
         _objFactory.destroy(filter);
     }
 
+    void destroyListener(EventListener listener)
+    {
+        _objFactory.destroy(listener);
+    }
+
     public static class JspPropertyGroup implements JspPropertyGroupDescriptor
     {
         private List<String> _urlPatterns = new ArrayList<String>();
@@ -1274,6 +1277,11 @@ public class ServletContextHandler extends ContextHandler
             }
         }
 
+        public <T extends Filter> void destroyFilter(T f)
+        {
+            _objFactory.destroy(f);
+        }
+
         @Override
         public <T extends Servlet> T createServlet(Class<T> c) throws ServletException
         {
@@ -1289,6 +1297,11 @@ public class ServletContextHandler extends ContextHandler
             }
         }
 
+        public <T extends Servlet> void destroyServlet(T s)
+        {
+            _objFactory.destroy(s);
+        }
+
         @Override
         public Set<SessionTrackingMode> getDefaultSessionTrackingModes()
         {

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

@@ -23,6 +23,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.EnumSet;
+import java.util.EventListener;
 import java.util.HashMap;
 import java.util.List;
 import java.util.ListIterator;
@@ -1706,6 +1707,12 @@ public class ServletHandler extends ScopedHandler
             _contextHandler.destroyFilter(filter);
     }
 
+    void destroyListener(EventListener listener)
+    {
+        if (_contextHandler != null)
+            _contextHandler.destroyListener(listener);
+    }
+
     @SuppressWarnings("serial")
     public static class Default404Servlet extends HttpServlet
     {

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

@@ -32,6 +32,8 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import javax.servlet.GenericServlet;
 import javax.servlet.MultipartConfigElement;
 import javax.servlet.Servlet;
 import javax.servlet.ServletConfig;
@@ -43,6 +45,7 @@ import javax.servlet.ServletResponse;
 import javax.servlet.ServletSecurityElement;
 import javax.servlet.SingleThreadModel;
 import javax.servlet.UnavailableException;
+import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.security.IdentityService;
 import org.eclipse.jetty.security.RunAsToken;
@@ -70,7 +73,6 @@ import org.eclipse.jetty.util.log.Logger;
 @ManagedObject("Servlet Holder")
 public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope, Comparable<ServletHolder>
 {
-
     private static final Logger LOG = Log.getLogger(ServletHolder.class);
     private int _initOrder = -1;
     private boolean _initOnStartup = false;
@@ -82,11 +84,9 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     private ServletRegistration.Dynamic _registration;
     private JspContainer _jspContainer;
 
-    private Servlet _servlet;
+    private volatile Servlet _servlet;
-    private long _unavailable;
     private Config _config;
     private boolean _enabled = true;
-    private UnavailableException _unavailableEx;
 
     public static final String APACHE_SENTINEL_CLASS = "org.apache.tomcat.InstanceManager";
     public static final String JSP_GENERATED_PACKAGE_NAME = "org.eclipse.jetty.servlet.jspPackagePrefix";
@@ -167,7 +167,10 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
      */
     public UnavailableException getUnavailableException()
     {
-        return _unavailableEx;
+        Servlet servlet = _servlet;
+        if (servlet instanceof UnavailableServlet)
+            return ((UnavailableServlet)servlet).getUnavailableException();
+        return null;
     }
 
     public synchronized void setServlet(Servlet servlet)
@@ -175,11 +178,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         if (servlet == null || servlet instanceof SingleThreadModel)
             throw new IllegalArgumentException(SingleThreadModel.class.getName() + " has been deprecated since Servlet API 2.4");
 
-        _extInstance = true;
+        setInstance(servlet);
-        _servlet = servlet;
-        setHeldClass(servlet.getClass());
-        if (getName() == null)
-            setName(servlet.getClass().getName() + "-" + super.hashCode());
     }
 
     @ManagedAttribute(value = "initialization order", readonly = true)
@@ -218,20 +217,20 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         if (sh._initOrder > _initOrder)
             return -1;
 
-        // consider _className, need to position properly when one is configured but not the other
+        // consider getClassName(), need to position properly when one is configured but not the other
         int c;
-        if (_className == null && sh._className == null)
+        if (getClassName() == null && sh.getClassName() == null)
             c = 0;
-        else if (_className == null)
+        else if (getClassName() == null)
             c = -1;
-        else if (sh._className == null)
+        else if (sh.getClassName() == null)
             c = 1;
         else
-            c = _className.compareTo(sh._className);
+            c = getClassName().compareTo(sh.getClassName());
 
-        // if _initOrder and _className are the same, consider the _name
+        // if _initOrder and getClassName() are the same, consider the getName()
         if (c == 0)
-            c = _name.compareTo(sh._name);
+            c = getName().compareTo(sh.getName());
 
         return c;
     }
@@ -245,7 +244,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     @Override
     public int hashCode()
     {
-        return _name == null ? System.identityHashCode(this) : _name.hashCode();
+        return getName() == null ? System.identityHashCode(this) : getName().hashCode();
     }
 
     /**
@@ -309,7 +308,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     public void doStart()
         throws Exception
     {
-        _unavailable = 0;
         if (!_enabled)
             return;
 
@@ -342,7 +340,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
                         //copy jsp init params that don't exist for this servlet
                         for (Map.Entry<String, String> entry : jsp.getInitParameters().entrySet())
                         {
-                            if (!_initParams.containsKey(entry.getKey()))
+                            if (!getInitParameters().containsKey(entry.getKey()))
                                 setInitParameter(entry.getKey(), entry.getValue());
                         }
                         //jsp specific: set up the jsp-file on the JspServlet. If load-on-startup is >=0 and the jsp container supports
@@ -365,7 +363,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         catch (UnavailableException ex)
         {
             makeUnavailable(ex);
-            if (_servletHandler.isStartWithUnavailable())
+            if (getServletHandler().isStartWithUnavailable())
             {
                 LOG.ignore(ex);
                 return;
@@ -382,7 +380,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         catch (UnavailableException ex)
         {
             makeUnavailable(ex);
-            if (_servletHandler.isStartWithUnavailable())
+            if (getServletHandler().isStartWithUnavailable())
             {
                 LOG.ignore(ex);
                 return;
@@ -394,16 +392,23 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         //check if we need to forcibly set load-on-startup
         checkInitOnStartup();
 
-        _identityService = _servletHandler.getIdentityService();
+        if (_runAsRole == null)
-        if (_identityService != null && _runAsRole != null)
+        {
-            _runAsToken = _identityService.newRunAsToken(_runAsRole);
+            _identityService = null;
+            _runAsToken = null;
+        }
+        else
+        {
+            _identityService = getServletHandler().getIdentityService();
+            if (_identityService != null)
+                _runAsToken = _identityService.newRunAsToken(_runAsRole);
+        }
 
         _config = new Config();
 
         synchronized (this)
         {
-            // TODO: remove support for deprecated SingleThreadModel??
+            if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass()))
-            if (_class != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(_class))
                 _servlet = new SingleThreadedWrapper();
         }
     }
@@ -412,57 +417,37 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     public void initialize()
         throws Exception
     {
-        if (!_initialized)
+        synchronized (this)
         {
-            super.initialize();
+            if (_servlet == null && (_initOnStartup || isInstance()))
-            if (_extInstance || _initOnStartup)
             {
-                try
+                super.initialize();
-                {
+                initServlet();
-                    initServlet();
-                }
-                catch (Exception e)
-                {
-                    if (_servletHandler.isStartWithUnavailable())
-                        LOG.ignore(e);
-                    else
-                        throw e;
-                }
             }
         }
-        _initialized = true;
     }
 
     @Override
     public void doStop()
         throws Exception
     {
-        Object oldRunAs = null;
+        synchronized (this)
-        if (_servlet != null)
         {
-            try
+            Servlet servlet = _servlet;
+            if (servlet != null)
             {
-                if (_identityService != null)
+                _servlet = null;
-                    oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken);
+                try
-
+                {
-                destroyInstance(_servlet);
+                    destroyInstance(servlet);
-            }
+                }
-            catch (Exception e)
+                catch (Exception e)
-            {
+                {
-                LOG.warn(e);
+                    LOG.warn(e);
-            }
+                }
-            finally
-            {
-                if (_identityService != null)
-                    _identityService.unsetRunAs(oldRunAs);
             }
+            _config = null;
         }
-
-        if (!_extInstance)
-            _servlet = null;
-
-        _config = null;
-        _initialized = false;
     }
 
     @Override
@@ -482,41 +467,24 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
      * @return The servlet
      * @throws ServletException if unable to init the servlet on first use
      */
-    public synchronized Servlet getServlet()
+    public Servlet getServlet()
         throws ServletException
     {
         Servlet servlet = _servlet;
-        if (servlet != null && _unavailable == 0)
+        if (servlet == null)
-            return servlet;
-
-        synchronized (this)
         {
-            // Handle previous unavailability
+            synchronized (this)
-            if (_unavailable != 0)
             {
-                if (_unavailable < 0 || _unavailable > 0 && System.currentTimeMillis() < _unavailable)
-                    throw _unavailableEx;
-                _unavailable = 0;
-                _unavailableEx = null;
-            }
-
-            servlet = _servlet;
-            if (servlet != null)
-                return servlet;
-
-            if (isRunning())
-            {
-                if (_class == null)
-                    throw new UnavailableException("Servlet Not Initialized");
-                if (_unavailable != 0 || !_initOnStartup)
-                    initServlet();
                 servlet = _servlet;
-                if (servlet == null)
+                if (servlet == null && isRunning())
-                    throw new UnavailableException("Could not instantiate " + _class);
+                {
+                    if (getHeldClass() != null)
+                        initServlet();
+                    servlet = _servlet;
+                }
             }
-
-            return servlet;
         }
+        return servlet;
     }
 
     /**
@@ -526,13 +494,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
      */
     public Servlet getServletInstance()
     {
-        Servlet servlet = _servlet;
+        return _servlet;
-        if (servlet != null)
-            return servlet;
-        synchronized (this)
-        {
-            return _servlet;
-        }
     }
 
     /**
@@ -543,9 +505,9 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     public void checkServletType()
         throws UnavailableException
     {
-        if (_class == null || !javax.servlet.Servlet.class.isAssignableFrom(_class))
+        if (getHeldClass() == null || !javax.servlet.Servlet.class.isAssignableFrom(getHeldClass()))
         {
-            throw new UnavailableException("Servlet " + _class + " is not a javax.servlet.Servlet");
+            throw new UnavailableException("Servlet " + getHeldClass() + " is not a javax.servlet.Servlet");
         }
     }
 
@@ -554,18 +516,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
      */
     public boolean isAvailable()
     {
-        if (isStarted() && _unavailable == 0)
+        return (isStarted() && !(_servlet instanceof UnavailableServlet));
-            return true;
-        try
-        {
-            getServlet();
-        }
-        catch (Exception e)
-        {
-            LOG.ignore(e);
-        }
-
-        return isStarted() && _unavailable == 0;
     }
 
     /**
@@ -576,30 +527,19 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
      */
     private void checkInitOnStartup()
     {
-        if (_class == null)
+        if (getHeldClass() == null)
             return;
 
-        if ((_class.getAnnotation(javax.servlet.annotation.ServletSecurity.class) != null) && !_initOnStartup)
+        if ((getHeldClass().getAnnotation(javax.servlet.annotation.ServletSecurity.class) != null) && !_initOnStartup)
             setInitOrder(Integer.MAX_VALUE);
     }
 
-    private void makeUnavailable(UnavailableException e)
+    private Servlet makeUnavailable(UnavailableException e)
     {
-        if (_unavailableEx == e && _unavailable != 0)
+        synchronized (this)
-            return;
-
-        _servletHandler.getServletContext().log("unavailable", e);
-
-        _unavailableEx = e;
-        _unavailable = -1;
-        if (e.isPermanent())
-            _unavailable = -1;
-        else
         {
-            if (_unavailableEx.getUnavailableSeconds() > 0)
+            _servlet = new UnavailableServlet(e, _servlet);
-                _unavailable = System.currentTimeMillis() + 1000 * _unavailableEx.getUnavailableSeconds();
+            return _servlet;
-            else
-                _unavailable = System.currentTimeMillis() + 5000; // TODO configure
         }
     }
 
@@ -609,37 +549,39 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
             makeUnavailable((UnavailableException)e);
         else
         {
-            ServletContext ctx = _servletHandler.getServletContext();
+            ServletContext ctx = getServletHandler().getServletContext();
             if (ctx == null)
                 LOG.info("unavailable", e);
             else
                 ctx.log("unavailable", e);
-            _unavailableEx = new UnavailableException(String.valueOf(e), -1)
+            UnavailableException unavailable = new UnavailableException(String.valueOf(e), -1)
             {
                 {
                     initCause(e);
                 }
             };
-            _unavailable = -1;
+            makeUnavailable(unavailable);
         }
     }
 
     private synchronized void initServlet()
         throws ServletException
     {
-        Object oldRunAs = null;
         try
         {
+            if (_servlet == null)
+                _servlet = getInstance();
             if (_servlet == null)
                 _servlet = newInstance();
             if (_config == null)
                 _config = new Config();
 
             // Handle run as
-            if (_identityService != null)
+            if (_identityService != null && _runAsToken != null)
-            {
+                _servlet = new RunAsServlet(_servlet, _identityService, _runAsToken);
-                oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken);
+
-            }
+            if (!isAsyncSupported())
+                _servlet = new NotAsyncServlet(_servlet);
 
             // Handle configuring servlets that implement org.apache.jasper.servlet.JspServlet
             if (isJspServlet())
@@ -659,30 +601,21 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         catch (UnavailableException e)
         {
             makeUnavailable(e);
-            _servlet = null;
+            if (getServletHandler().isStartWithUnavailable())
-            _config = null;
+                LOG.warn(e);
-            throw e;
+            else
+                throw e;
         }
         catch (ServletException e)
         {
             makeUnavailable(e.getCause() == null ? e : e.getCause());
-            _servlet = null;
-            _config = null;
             throw e;
         }
         catch (Exception e)
         {
             makeUnavailable(e);
-            _servlet = null;
-            _config = null;
             throw new ServletException(this.toString(), e);
         }
-        finally
-        {
-            // pop run-as role
-            if (_identityService != null)
-                _identityService.unsetRunAs(oldRunAs);
-        }
     }
 
     /**
@@ -693,7 +626,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         ContextHandler ch = ContextHandler.getContextHandler(getServletHandler().getServletContext());
 
         /* Set the webapp's classpath for Jasper */
-        ch.setAttribute("org.apache.catalina.jsp_classpath", ch.getClassPath());
+        ch.setAttribute("org.apache.catalina.jspgetHeldClass()path", ch.getClassPath());
 
         /* Set up other classpath attribute */
         if ("?".equals(getInitParameter("classpath")))
@@ -812,56 +745,23 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         UnavailableException,
         IOException
     {
-        if (_class == null)
-            throw new UnavailableException("Servlet Not Initialized");
-
-        Servlet servlet = getServlet();
-
-        // Service the request
-        Object oldRunAs = null;
-        boolean suspendable = baseRequest.isAsyncSupported();
         try
         {
-            // Handle aliased path
+            Servlet servlet = getServlet();
-            if (_forcedPath != null)
+            if (servlet == null)
-                adaptForcedPathToJspContainer(request);
+                throw new UnavailableException("Servlet Not Initialized");
-
+            servlet.service(request, response);
-            // Handle run as
-            if (_identityService != null)
-                oldRunAs = _identityService.setRunAs(baseRequest.getResolvedUserIdentity(), _runAsToken);
-
-            if (baseRequest.isAsyncSupported() && !isAsyncSupported())
-            {
-                try
-                {
-                    baseRequest.setAsyncSupported(false, this.toString());
-                    servlet.service(request, response);
-                }
-                finally
-                {
-                    baseRequest.setAsyncSupported(true, null);
-                }
-            }
-            else
-                servlet.service(request, response);
         }
         catch (UnavailableException e)
         {
-            makeUnavailable(e);
+            makeUnavailable(e).service(request, response);
-            throw _unavailableEx;
-        }
-        finally
-        {
-            // Pop run-as role.
-            if (_identityService != null)
-                _identityService.unsetRunAs(oldRunAs);
         }
     }
 
     protected boolean isJspServlet()
     {
         Servlet servlet = getServletInstance();
-        Class<?> c = servlet == null ? _class : servlet.getClass();
+        Class<?> c = servlet == null ? getHeldClass() : servlet.getClass();
 
         while (c != null)
         {
@@ -879,11 +779,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         return ("org.apache.jasper.servlet.JspServlet".equals(classname));
     }
 
-    private void adaptForcedPathToJspContainer(ServletRequest request)
-    {
-        //no-op for apache jsp
-    }
-
     private void detectJspContainer()
     {
         if (_jspContainer == null)
@@ -1051,7 +946,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
             Set<String> clash = null;
             for (String pattern : urlPatterns)
             {
-                ServletMapping mapping = _servletHandler.getServletMapping(pattern);
+                ServletMapping mapping = getServletHandler().getServletMapping(pattern);
                 if (mapping != null)
                 {
                     //if the servlet mapping was from a default descriptor, then allow it to be overridden
@@ -1072,7 +967,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
             ServletMapping mapping = new ServletMapping(Source.JAVAX_API);
             mapping.setServletName(ServletHolder.this.getName());
             mapping.setPathSpecs(urlPatterns);
-            _servletHandler.addServletMapping(mapping);
+            getServletHandler().addServletMapping(mapping);
 
             return Collections.emptySet();
         }
@@ -1080,7 +975,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         @Override
         public Collection<String> getMappings()
         {
-            ServletMapping[] mappings = _servletHandler.getServletMappings();
+            ServletMapping[] mappings = getServletHandler().getServletMappings();
             List<String> patterns = new ArrayList<String>();
             if (mappings != null)
             {
@@ -1134,7 +1029,7 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
         @Override
         public Set<String> setServletSecurity(ServletSecurityElement securityElement)
         {
-            return _servletHandler.setServletSecurity(this, securityElement);
+            return getServletHandler().setServletSecurity(this, securityElement);
         }
     }
 
@@ -1282,18 +1177,221 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
     @Override
     public void dump(Appendable out, String indent) throws IOException
     {
-        if (_initParams.isEmpty())
+        if (getInitParameters().isEmpty())
             Dumpable.dumpObjects(out, indent, this,
                 _servlet == null ? getHeldClass() : _servlet);
         else
             Dumpable.dumpObjects(out, indent, this,
                 _servlet == null ? getHeldClass() : _servlet,
-                new DumpableCollection("initParams", _initParams.entrySet()));
+                new DumpableCollection("initParams", getInitParameters().entrySet()));
     }
 
     @Override
     public String toString()
     {
-        return String.format("%s@%x==%s,jsp=%s,order=%d,inst=%b,async=%b", _name, hashCode(), _className, _forcedPath, _initOrder, _servlet != null, isAsyncSupported());
+        return String.format("%s@%x==%s,jsp=%s,order=%d,inst=%b,async=%b", getName(), hashCode(), getClassName(), _forcedPath, _initOrder, _servlet != null, isAsyncSupported());
+    }
+
+    private class UnavailableServlet extends GenericServlet
+    {
+        final UnavailableException _unavailableException;
+        final Servlet _servlet;
+        final long _available;
+
+        public UnavailableServlet(UnavailableException unavailableException, Servlet servlet)
+        {
+            _unavailableException = unavailableException;
+
+            if (unavailableException.isPermanent())
+            {
+                _servlet = null;
+                _available = -1;
+                if (servlet != null)
+                {
+                    try
+                    {
+                        destroyInstance(servlet);
+                    }
+                    catch (Throwable th)
+                    {
+                        if (th != unavailableException)
+                            unavailableException.addSuppressed(th);
+                    }
+                }
+            }
+            else
+            {
+                _servlet = servlet;
+                _available = System.nanoTime() + TimeUnit.SECONDS.toNanos(unavailableException.getUnavailableSeconds());
+            }
+        }
+
+        @Override
+        public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
+        {
+            if (_available == -1)
+                ((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND);
+            else if (System.nanoTime() < _available)
+                ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
+            else
+            {
+                synchronized (ServletHolder.this)
+                {
+                    ServletHolder.this._servlet = this._servlet;
+                    _servlet.service(req, res);
+                }
+            }
+        }
+
+        @Override
+        public void destroy()
+        {
+            if (_servlet != null)
+            {
+                try
+                {
+                    destroyInstance(_servlet);
+                }
+                catch (Throwable th)
+                {
+                    LOG.warn(th);
+                }
+            }
+        }
+
+        public UnavailableException getUnavailableException()
+        {
+            return _unavailableException;
+        }
+    }
+
+    private static class WrapperServlet implements Servlet
+    {
+        final Servlet _servlet;
+
+        public WrapperServlet(Servlet servlet)
+        {
+            _servlet = servlet;
+        }
+
+        @Override
+        public void init(ServletConfig config) throws ServletException
+        {
+            _servlet.init(config);
+        }
+
+        @Override
+        public ServletConfig getServletConfig()
+        {
+            return _servlet.getServletConfig();
+        }
+
+        @Override
+        public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
+        {
+            _servlet.service(req, res);
+        }
+
+        @Override
+        public String getServletInfo()
+        {
+            return _servlet.getServletInfo();
+        }
+
+        @Override
+        public void destroy()
+        {
+            _servlet.destroy();
+        }
+
+        @Override
+        public String toString()
+        {
+            return String.format("%s:%s", this.getClass().getSimpleName(), _servlet.toString());
+        }
+    }
+
+    private static class RunAsServlet extends WrapperServlet
+    {
+        final IdentityService _identityService;
+        final RunAsToken _runAsToken;
+
+        public RunAsServlet(Servlet servlet, IdentityService identityService, RunAsToken runAsToken)
+        {
+            super(servlet);
+            _identityService = identityService;
+            _runAsToken = runAsToken;
+        }
+
+        @Override
+        public void init(ServletConfig config) throws ServletException
+        {
+            Object oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken);
+            try
+            {
+                _servlet.init(config);
+            }
+            finally
+            {
+                _identityService.unsetRunAs(oldRunAs);
+            }
+        }
+
+        @Override
+        public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
+        {
+            Object oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken);
+            try
+            {
+                _servlet.service(req, res);
+            }
+            finally
+            {
+                _identityService.unsetRunAs(oldRunAs);
+            }
+        }
+
+        @Override
+        public void destroy()
+        {
+            Object oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken);
+            try
+            {
+                _servlet.destroy();
+            }
+            finally
+            {
+                _identityService.unsetRunAs(oldRunAs);
+            }
+        }
+    }
+
+    private static class NotAsyncServlet extends WrapperServlet
+    {
+        public NotAsyncServlet(Servlet servlet)
+        {
+            super(servlet);
+        }
+
+        @Override
+        public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
+        {
+            if (req.isAsyncSupported())
+            {
+                try
+                {
+                    ((Request)req).setAsyncSupported(false, this.toString());
+                    _servlet.service(req, res);
+                }
+                finally
+                {
+                    ((Request)req).setAsyncSupported(true, null);
+                }
+            }
+            else
+            {
+                _servlet.service(req, res);
+            }
+        }
     }
 }

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

@@ -26,6 +26,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import javax.servlet.AsyncContext;
 import javax.servlet.DispatcherType;
@@ -36,6 +37,7 @@ import javax.servlet.Servlet;
 import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
+import javax.servlet.UnavailableException;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -58,6 +60,7 @@ import org.junit.jupiter.api.Test;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.not;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ErrorPageTest
@@ -67,6 +70,8 @@ public class ErrorPageTest
     private StacklessLogging _stackless;
     private static CountDownLatch __asyncSendErrorCompleted;
     private ErrorPageErrorHandler _errorPageErrorHandler;
+    private static AtomicBoolean __destroyed;
+    private ServletContextHandler _context;
 
     @BeforeEach
     public void init() throws Exception
@@ -75,25 +80,24 @@ public class ErrorPageTest
         _connector = new LocalConnector(_server);
         _server.addConnector(_connector);
 
-        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS);
+        _context = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS);
 
-        _server.setHandler(context);
+        _server.setHandler(_context);
 
-        context.setContextPath("/");
+        _context.setContextPath("/");
-
+        _context.addFilter(SingleDispatchFilter.class, "/*", EnumSet.allOf(DispatcherType.class));
-        context.addFilter(SingleDispatchFilter.class, "/*", EnumSet.allOf(DispatcherType.class));
+        _context.addServlet(DefaultServlet.class, "/");
-
+        _context.addServlet(FailServlet.class, "/fail/*");
-        context.addServlet(DefaultServlet.class, "/");
+        _context.addServlet(FailClosedServlet.class, "/fail-closed/*");
-        context.addServlet(FailServlet.class, "/fail/*");
+        _context.addServlet(ErrorServlet.class, "/error/*");
-        context.addServlet(FailClosedServlet.class, "/fail-closed/*");
+        _context.addServlet(AppServlet.class, "/app/*");
-        context.addServlet(ErrorServlet.class, "/error/*");
+        _context.addServlet(LongerAppServlet.class, "/longer.app/*");
-        context.addServlet(AppServlet.class, "/app/*");
+        _context.addServlet(SyncSendErrorServlet.class, "/sync/*");
-        context.addServlet(LongerAppServlet.class, "/longer.app/*");
+        _context.addServlet(AsyncSendErrorServlet.class, "/async/*");
-        context.addServlet(SyncSendErrorServlet.class, "/sync/*");
+        _context.addServlet(NotEnoughServlet.class, "/notenough/*");
-        context.addServlet(AsyncSendErrorServlet.class, "/async/*");
+        _context.addServlet(UnavailableServlet.class, "/unavailable/*");
-        context.addServlet(NotEnoughServlet.class, "/notenough/*");
+        _context.addServlet(DeleteServlet.class, "/delete/*");
-        context.addServlet(DeleteServlet.class, "/delete/*");
+        _context.addServlet(ErrorAndStatusServlet.class, "/error-and-status/*");
-        context.addServlet(ErrorAndStatusServlet.class, "/error-and-status/*");
 
         HandlerWrapper noopHandler = new HandlerWrapper()
         {
@@ -106,10 +110,10 @@ public class ErrorPageTest
                     super.handle(target, baseRequest, request, response);
             }
         };
-        context.insertHandler(noopHandler);
+        _context.insertHandler(noopHandler);
 
         _errorPageErrorHandler = new ErrorPageErrorHandler();
-        context.setErrorHandler(_errorPageErrorHandler);
+        _context.setErrorHandler(_errorPageErrorHandler);
         _errorPageErrorHandler.addErrorPage(595, "/error/595");
         _errorPageErrorHandler.addErrorPage(597, "/sync");
         _errorPageErrorHandler.addErrorPage(599, "/error/599");
@@ -408,6 +412,46 @@ public class ErrorPageTest
         assertThat(response, Matchers.endsWith("SomeBytes"));
     }
 
+    @Test
+    public void testPermanentlyUnavailable() throws Exception
+    {
+        try (StacklessLogging ignore = new StacklessLogging(_context.getLogger()))
+        {
+            try (StacklessLogging ignore2 = new StacklessLogging(HttpChannel.class))
+            {
+                __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 "));
+                assertTrue(__destroyed.get());
+            }
+        }
+    }
+
+    @Test
+    public void testUnavailable() throws Exception
+    {
+        try (StacklessLogging ignore = new StacklessLogging(_context.getLogger()))
+        {
+            try (StacklessLogging ignore2 = new StacklessLogging(HttpChannel.class))
+            {
+                __destroyed = new AtomicBoolean(false);
+                String response = _connector.getResponse("GET /unavailable/info?for=1 HTTP/1.0\r\n\r\n");
+                assertThat(response, Matchers.containsString("HTTP/1.1 503 "));
+                assertFalse(__destroyed.get());
+
+                response = _connector.getResponse("GET /unavailable/info?ok=true HTTP/1.0\r\n\r\n");
+                assertThat(response, Matchers.containsString("HTTP/1.1 503 "));
+                assertFalse(__destroyed.get());
+
+                Thread.sleep(1500);
+
+                response = _connector.getResponse("GET /unavailable/info?ok=true HTTP/1.0\r\n\r\n");
+                assertThat(response, Matchers.containsString("HTTP/1.1 200 "));
+                assertFalse(__destroyed.get());
+            }
+        }
+    }
+
     public static class AppServlet extends HttpServlet implements Servlet
     {
         @Override
@@ -618,6 +662,34 @@ public class ErrorPageTest
         }
     }
 
+    public static class UnavailableServlet extends HttpServlet implements Servlet
+    {
+        @Override
+        protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        {
+            String ok = request.getParameter("ok");
+            if (Boolean.parseBoolean(ok))
+            {
+                response.setStatus(200);
+                response.flushBuffer();
+                return;
+            }
+
+            String f = request.getParameter("for");
+            if (f == null)
+                throw new UnavailableException("testing permanent");
+
+            throw new UnavailableException("testing periodic", Integer.parseInt(f));
+        }
+
+        @Override
+        public void destroy()
+        {
+            if (__destroyed != null)
+                __destroyed.set(true);
+        }
+    }
+
     public static class SingleDispatchFilter implements Filter
     {
         ConcurrentMap<Integer, Thread> dispatches = new ConcurrentHashMap<>();
@@ -665,7 +737,6 @@ public class ErrorPageTest
         @Override
         public void destroy()
         {
-
         }
     }
 }

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

@@ -1,80 +0,0 @@
-//
-//  ========================================================================
-//  Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd.
-//  ------------------------------------------------------------------------
-//  All rights reserved. This program and the accompanying materials
-//  are made available under the terms of the Eclipse Public License v1.0
-//  and Apache License v2.0 which accompanies this distribution.
-//
-//      The Eclipse Public License is available at
-//      http://www.eclipse.org/legal/epl-v10.html
-//
-//      The Apache License v2.0 is available at
-//      http://www.opensource.org/licenses/apache2.0.php
-//
-//  You may elect to redistribute this code under either of these licenses.
-//  ========================================================================
-//
-
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- *  http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package org.eclipse.jetty.servlet;
-
-import java.util.Collections;
-import java.util.Set;
-import javax.servlet.ServletRegistration;
-
-import org.junit.jupiter.api.Test;
-
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
-/**
- *
- */
-public class HolderTest
-{
-
-    @Test
-    public void testInitParams() throws Exception
-    {
-        ServletHolder holder = new ServletHolder(Source.JAVAX_API);
-        ServletRegistration reg = holder.getRegistration();
-
-        assertThrows(IllegalArgumentException.class,() ->  reg.setInitParameter(null, "foo"));
-
-        assertThrows(IllegalArgumentException.class,() -> reg.setInitParameter("foo", null));
-
-        reg.setInitParameter("foo", "bar");
-        assertFalse(reg.setInitParameter("foo", "foo"));
-
-        Set<String> clash = reg.setInitParameters(Collections.singletonMap("foo", "bax"));
-        assertTrue(clash != null && clash.size() == 1, "should be one clash");
-
-        assertThrows(IllegalArgumentException.class,() ->  reg.setInitParameters(Collections.singletonMap((String)null, "bax")));
-        assertThrows(IllegalArgumentException.class,() ->  reg.setInitParameters(Collections.singletonMap("foo", (String)null)));
-
-        Set<String> clash2 = reg.setInitParameters(Collections.singletonMap("FOO", "bax"));
-        assertTrue(clash2.isEmpty(), "should be no clash");
-        assertEquals(2, reg.getInitParameters().size(), "setInitParameters should not replace existing non-clashing init parameters");
-    }
-}

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

@@ -18,11 +18,11 @@
 
 package org.eclipse.jetty.servlet;
 
-import javax.servlet.ServletException;
+import java.util.Collections;
+import java.util.Set;
+import javax.servlet.ServletRegistration;
 import javax.servlet.UnavailableException;
 import javax.servlet.http.HttpServlet;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.util.MultiException;
@@ -32,18 +32,41 @@ import org.junit.jupiter.api.Test;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
-import java.io.IOException;
-
 public class ServletHolderTest
 {
-    
     public static class FakeServlet extends HttpServlet
     {
     }
-    
+
+    @Test
+    public void testInitParams() throws Exception
+    {
+        ServletHolder holder = new ServletHolder(Source.JAVAX_API);
+        ServletRegistration reg = holder.getRegistration();
+
+        assertThrows(IllegalArgumentException.class, () -> reg.setInitParameter(null, "foo"));
+
+        assertThrows(IllegalArgumentException.class, () -> reg.setInitParameter("foo", null));
+
+        reg.setInitParameter("foo", "bar");
+        assertFalse(reg.setInitParameter("foo", "foo"));
+
+        Set<String> clash = reg.setInitParameters(Collections.singletonMap("foo", "bax"));
+        assertTrue(clash != null && clash.size() == 1, "should be one clash");
+
+        assertThrows(IllegalArgumentException.class, () -> reg.setInitParameters(Collections.singletonMap((String)null, "bax")));
+        assertThrows(IllegalArgumentException.class, () -> reg.setInitParameters(Collections.singletonMap("foo", (String)null)));
+
+        Set<String> clash2 = reg.setInitParameters(Collections.singletonMap("FOO", "bax"));
+        assertTrue(clash2.isEmpty(), "should be no clash");
+        assertEquals(2, reg.getInitParameters().size(), "setInitParameters should not replace existing non-clashing init parameters");
+    }
 
     @Test
     public void testTransitiveCompareTo() throws Exception
@@ -78,26 +101,16 @@ public class ServletHolderTest
         ServletHolder h = new ServletHolder();
         h.setName("test");
 
-        assertEquals(null, h.getClassNameForJsp(null));
+        assertNull(h.getClassNameForJsp(null));
-
+        assertNull(h.getClassNameForJsp(""));
-        assertEquals(null, h.getClassNameForJsp(""));
+        assertNull(h.getClassNameForJsp("/blah/"));
-
+        assertNull(h.getClassNameForJsp("//blah///"));
-        assertEquals(null, h.getClassNameForJsp("/blah/"));
+        assertNull(h.getClassNameForJsp("/a/b/c/blah/"));
-
-        assertEquals(null, h.getClassNameForJsp("//blah///"));
-
-        assertEquals(null, h.getClassNameForJsp("/a/b/c/blah/"));
-
         assertEquals("org.apache.jsp.a.b.c.blah", h.getClassNameForJsp("/a/b/c/blah"));
-
         assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("/blah.jsp"));
-
         assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("//blah.jsp"));
-
         assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("blah.jsp"));
-
         assertEquals("org.apache.jsp.a.b.c.blah_jsp", h.getClassNameForJsp("/a/b/c/blah.jsp"));
-
         assertEquals("org.apache.jsp.a.b.c.blah_jsp", h.getClassNameForJsp("a/b/c/blah.jsp"));
     }
 

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

@@ -0,0 +1,229 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.servlet;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.EventListener;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import javax.servlet.DispatcherType;
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.Servlet;
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletContextEvent;
+import javax.servlet.ServletContextListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.util.Decorator;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+public class ServletLifeCycleTest
+{
+    static final Queue<String> events = new ConcurrentLinkedQueue<>();
+
+    @Test
+    public void testLifeCycle() throws Exception
+    {
+        Server server = new Server(0);
+        LocalConnector connector = new LocalConnector(server);
+        server.addConnector(connector);
+
+        ServletContextHandler context = new ServletContextHandler(server, "/");
+
+        context.getObjectFactory().addDecorator(new TestDecorator());
+
+        ServletHandler sh = context.getServletHandler();
+        sh.addListener(new ListenerHolder(TestListener.class));
+        context.addEventListener(context.getServletContext().createListener(TestListener2.class));
+
+        sh.addFilterWithMapping(TestFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
+        sh.addFilterWithMapping(new FilterHolder(context.getServletContext().createFilter(TestFilter2.class)), "/*", EnumSet.of(DispatcherType.REQUEST));
+
+        sh.addServletWithMapping(TestServlet.class, "/1/*").setInitOrder(1);
+        sh.addServletWithMapping(TestServlet2.class, "/2/*").setInitOrder(-1);
+        sh.addServletWithMapping(new ServletHolder(context.getServletContext().createServlet(TestServlet3.class))
+        {{
+            setInitOrder(1);
+        }}, "/3/*");
+
+        assertThat(events, Matchers.contains(
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener2",
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter2",
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet3"));
+
+        events.clear();
+        server.start();
+        assertThat(events, Matchers.contains(
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener",
+            "ContextInitialized class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener2",
+            "ContextInitialized class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener",
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter",
+            "init class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter",
+            "init class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter2",
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet",
+            "init class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet",
+            "init class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet3"));
+
+        events.clear();
+        connector.getResponse("GET /2/info HTTP/1.0\r\n\r\n");
+
+        assertThat(events, Matchers.contains(
+            "Decorate class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet2",
+            "init class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet2",
+            "doFilter class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter",
+            "doFilter class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter2",
+            "service class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet2"));
+
+        events.clear();
+        server.stop();
+
+        assertThat(events, Matchers.contains(
+            "contextDestroyed class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener",
+            "contextDestroyed class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener2",
+            "destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter2",
+            "Destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter2",
+            "destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter",
+            "Destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestFilter",
+            "Destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet3",
+            "destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet3",
+            "Destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet2",
+            "destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet2",
+            "Destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet",
+            "destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestServlet",
+            "Destroy class org.eclipse.jetty.servlet.ServletLifeCycleTest$TestListener"));
+
+        // Listener added before start is not destroyed
+        EventListener[] listeners = context.getEventListeners();
+        assertThat(listeners.length, is(1));
+        assertThat(listeners[0].getClass(), is(TestListener2.class));
+    }
+
+    public static class TestDecorator implements Decorator
+    {
+        @Override
+        public <T> T decorate(T o)
+        {
+            events.add("Decorate " + o.getClass());
+            return o;
+        }
+
+        @Override
+        public void destroy(Object o)
+        {
+            events.add("Destroy " + o.getClass());
+        }
+    }
+
+    public static class TestListener implements ServletContextListener
+    {
+        @Override
+        public void contextInitialized(ServletContextEvent sce)
+        {
+            events.add("ContextInitialized " + this.getClass());
+        }
+
+        @Override
+        public void contextDestroyed(ServletContextEvent sce)
+        {
+            events.add("contextDestroyed " + this.getClass());
+        }
+    }
+
+    public static class TestListener2 extends TestListener
+    {
+    }
+
+    public static class TestFilter implements Filter
+    {
+        @Override
+        public void init(FilterConfig filterConfig) throws ServletException
+        {
+            events.add("init " + this.getClass());
+        }
+
+        @Override
+        public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
+        {
+            events.add("doFilter " + this.getClass());
+            chain.doFilter(request, response);
+        }
+
+        @Override
+        public void destroy()
+        {
+            events.add("destroy " + this.getClass());
+        }
+    }
+
+    public static class TestFilter2 extends TestFilter
+    {
+    }
+
+    public static class TestServlet implements Servlet
+    {
+        @Override
+        public void init(ServletConfig config) throws ServletException
+        {
+            events.add("init " + this.getClass());
+        }
+
+        @Override
+        public ServletConfig getServletConfig()
+        {
+            return null;
+        }
+
+        @Override
+        public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
+        {
+            events.add("service " + this.getClass());
+        }
+
+        @Override
+        public String getServletInfo()
+        {
+            return null;
+        }
+
+        @Override
+        public void destroy()
+        {
+            events.add("destroy " + this.getClass());
+        }
+    }
+
+    public static class TestServlet2 extends TestServlet
+    {
+    }
+
+    public static class TestServlet3 extends TestServlet
+    {
+    }
+}