From d48c258deb54af200636311240a513cf1665354c Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 12 Sep 2019 17:56:09 +1000 Subject: [PATCH 1/2] Issue #97 ServletHolder unavailable handling Took the opportunity to do a major cleanup of the Holders. ServletHolder now uses wrapping to achieve optional behaviour rather than lots of if statements. Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/servlet/BaseHolder.java | 30 +- .../eclipse/jetty/servlet/FilterHolder.java | 57 +- .../org/eclipse/jetty/servlet/Holder.java | 23 +- .../eclipse/jetty/servlet/ListenerHolder.java | 64 ++- .../jetty/servlet/ServletContextHandler.java | 15 + .../eclipse/jetty/servlet/ServletHandler.java | 7 + .../eclipse/jetty/servlet/ServletHolder.java | 501 +++++++++++------- .../eclipse/jetty/servlet/ErrorPageTest.java | 111 +++- .../org/eclipse/jetty/servlet/HolderTest.java | 80 --- .../jetty/servlet/ServletHolderTest.java | 57 +- 10 files changed, 548 insertions(+), 397 deletions(-) delete mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/HolderTest.java diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java index ddcaa278423..5c688222192 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java @@ -43,11 +43,11 @@ public abstract class BaseHolder extends AbstractLifeCycle implements Dumpabl { private static final Logger LOG = Log.getLogger(BaseHolder.class); - protected final Source _source; - protected transient Class _class; - protected String _className; - protected boolean _extInstance; - protected ServletHandler _servletHandler; + private final Source _source; + private Class _class; + private String _className; + private T _instance; + private ServletHandler _servletHandler; protected BaseHolder(Source source) { @@ -101,7 +101,7 @@ public abstract class BaseHolder extends AbstractLifeCycle implements Dumpabl public void doStop() throws Exception { - if (!_extInstance) + if (_instance == null) _class = null; } @@ -163,12 +163,26 @@ public abstract class BaseHolder extends AbstractLifeCycle implements Dumpabl } } + protected synchronized void setInstance(T instance) + { + _instance = instance; + if (instance == null) + setHeldClass(null); + else + setHeldClass((Class)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 diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java index 3881bb05ff3..9b0f0b6261a 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java @@ -91,11 +91,10 @@ public class FilterHolder extends Holder { super.doStart(); - if (!javax.servlet.Filter.class - .isAssignableFrom(_class)) + if (!javax.servlet.Filter.class.isAssignableFrom(getHeldClass())) { - String msg = _class + " is not a javax.servlet.Filter"; - super.stop(); + String msg = getHeldClass() + " is not a javax.servlet.Filter"; + doStop(); throw new IllegalStateException(msg); } } @@ -103,15 +102,18 @@ public class FilterHolder extends Holder @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 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 public synchronized void setFilter(Filter filter) { - _filter = filter; - _extInstance = true; - setHeldClass(filter.getClass()); - if (getName() == null) - setName(filter.getClass().getName()); + setInstance(filter); } public Filter getFilter() @@ -187,19 +178,19 @@ public class FilterHolder extends Holder @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 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 mapping.setPathSpecs(urlPatterns); mapping.setDispatcherTypes(dispatcherTypes); if (isMatchAfter) - _servletHandler.addFilterMapping(mapping); + getServletHandler().addFilterMapping(mapping); else - _servletHandler.prependFilterMapping(mapping); + getServletHandler().prependFilterMapping(mapping); } @Override public Collection getServletNameMappings() { - FilterMapping[] mappings = _servletHandler.getFilterMappings(); + FilterMapping[] mappings = getServletHandler().getFilterMappings(); List names = new ArrayList(); for (FilterMapping mapping : mappings) { @@ -258,7 +249,7 @@ public class FilterHolder extends Holder @Override public Collection getUrlPatternMappings() { - FilterMapping[] mappings = _servletHandler.getFilterMappings(); + FilterMapping[] mappings = getServletHandler().getFilterMappings(); List patterns = new ArrayList(); for (FilterMapping mapping : mappings) { @@ -277,7 +268,7 @@ public class FilterHolder extends Holder @Override public String getFilterName() { - return _name; + return getName(); } } } diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java index 07010fd9192..7ee8392b5c1 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java @@ -45,16 +45,15 @@ public abstract class Holder extends BaseHolder { private static final Logger LOG = Log.getLogger(Holder.class); - protected final Map _initParams = new HashMap(3); - protected String _displayName; - protected boolean _asyncSupported; - protected String _name; - protected boolean _initialized = false; + private final Map _initParams = new HashMap(3); + private String _displayName; + private boolean _asyncSupported; + private String _name; 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 extends BaseHolder 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 extends BaseHolder @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 extends BaseHolder public ServletContext getServletContext() { - return _servletHandler.getServletContext(); + return getServletHandler().getServletContext(); } public String getInitParameter(String param) 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 245101dd63b..9313237ea4e 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 @@ -64,52 +64,66 @@ public class ListenerHolder extends BaseHolder */ public void setListener(EventListener listener) { - _listener = listener; - _extInstance = true; - setHeldClass(_listener.getClass()); + setInstance(listener); } @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 - try + _listener = getInstance(); + if (_listener == null) { - ServletContext scontext = contextHandler.getServletContext(); - _listener = (scontext instanceof ServletContextHandler.Context) - ? scontext.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; + //create an instance of the listener and decorate it + try + { + ServletContext scontext = contextHandler.getServletContext(); + _listener = (scontext instanceof ServletContextHandler.Context) + ? scontext.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; + } } + contextHandler.addEventListener(_listener); } - contextHandler.addEventListener(_listener); } @Override public void doStop() throws Exception { super.doStop(); - if (!_extInstance) - _listener = null; + if (_listener != null) + { + try + { + ContextHandler contextHandler = ContextHandler.getCurrentContext().getContextHandler(); + if (contextHandler != null) + contextHandler.removeEventListener(_listener); + getServletHandler().destroyListener(_listener); + } + finally + { + _listener = null; + } + } } @Override diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java index db28cfdbdb7..a2b790839d0 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java @@ -733,6 +733,11 @@ public class ServletContextHandler extends ContextHandler _objFactory.destroy(filter); } + void destroyListener(EventListener listener) + { + _objFactory.destroy(listener); + } + public static ServletContextHandler getServletContextHandler(ServletContext context) { ContextHandler handler = getContextHandler(context); @@ -1286,6 +1291,11 @@ public class ServletContextHandler extends ContextHandler } } + public void destroyFilter(T f) + { + _objFactory.destroy(f); + } + @Override public T createServlet(Class c) throws ServletException { @@ -1301,6 +1311,11 @@ public class ServletContextHandler extends ContextHandler } } + public void destroyServlet(T s) + { + _objFactory.destroy(s); + } + @Override public Set getDefaultSessionTrackingModes() { 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 897a9ccba98..df9f9249b1c 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 @@ -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; @@ -1736,6 +1737,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 { diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java index 9fb2b7e4ff6..6ba6598ffca 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java +++ b/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 implements UserIdentity.Scope, Comparable { - 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 implements UserIdentity.Scope private ServletRegistration.Dynamic _registration; private JspContainer _jspContainer; - private Servlet _servlet; - private long _unavailable; + private volatile Servlet _servlet; 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 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 implements UserIdentity.Scope if (servlet == null || servlet instanceof SingleThreadModel) throw new IllegalArgumentException(); - _extInstance = true; - _servlet = servlet; - setHeldClass(servlet.getClass()); - if (getName() == null) - setName(servlet.getClass().getName() + "-" + super.hashCode()); + setInstance(servlet); } @ManagedAttribute(value = "initialization order", readonly = true) @@ -218,20 +217,20 @@ public class ServletHolder extends Holder 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 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 implements UserIdentity.Scope public void doStart() throws Exception { - _unavailable = 0; if (!_enabled) return; @@ -342,7 +340,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope //copy jsp init params that don't exist for this servlet for (Map.Entry 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 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 implements UserIdentity.Scope catch (UnavailableException ex) { makeUnavailable(ex); - if (_servletHandler.isStartWithUnavailable()) + if (getServletHandler().isStartWithUnavailable()) { LOG.ignore(ex); return; @@ -394,15 +392,23 @@ public class ServletHolder extends Holder implements UserIdentity.Scope //check if we need to forcibly set load-on-startup checkInitOnStartup(); - _identityService = _servletHandler.getIdentityService(); - if (_identityService != null && _runAsRole != null) - _runAsToken = _identityService.newRunAsToken(_runAsRole); + if (_runAsRole == null) + { + _identityService = null; + _runAsToken = null; + } + else + { + _identityService = getServletHandler().getIdentityService(); + if (_identityService != null) + _runAsToken = _identityService.newRunAsToken(_runAsRole); + } _config = new Config(); synchronized (this) { - if (_class != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(_class)) + if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass())) _servlet = new SingleThreadedWrapper(); } } @@ -411,57 +417,37 @@ public class ServletHolder extends Holder implements UserIdentity.Scope public void initialize() throws Exception { - if (!_initialized) + synchronized (this) { - super.initialize(); - if (_extInstance || _initOnStartup) + if (_servlet == null && (_initOnStartup || isInstance())) { - try - { - initServlet(); - } - catch (Exception e) - { - if (_servletHandler.isStartWithUnavailable()) - LOG.ignore(e); - else - throw e; - } + super.initialize(); + initServlet(); } } - _initialized = true; } @Override public void doStop() throws Exception { - Object oldRunAs = null; - if (_servlet != null) + synchronized (this) { - try + Servlet servlet = _servlet; + if (servlet != null) { - if (_identityService != null) - oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken); - - destroyInstance(_servlet); - } - catch (Exception e) - { - LOG.warn(e); - } - finally - { - if (_identityService != null) - _identityService.unsetRunAs(oldRunAs); + _servlet = null; + try + { + destroyInstance(servlet); + } + catch (Exception e) + { + LOG.warn(e); + } } + _config = null; } - - if (!_extInstance) - _servlet = null; - - _config = null; - _initialized = false; } @Override @@ -481,41 +467,24 @@ public class ServletHolder extends Holder 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) - return servlet; - - synchronized (this) + if (servlet == null) { - // Handle previous unavailability - if (_unavailable != 0) + synchronized (this) { - 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) - throw new UnavailableException("Could not instantiate " + _class); + if (servlet == null && isRunning()) + { + if (getHeldClass() != null) + initServlet(); + servlet = _servlet; + } } - - return servlet; } + return servlet; } /** @@ -525,13 +494,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope */ public Servlet getServletInstance() { - Servlet servlet = _servlet; - if (servlet != null) - return servlet; - synchronized (this) - { - return _servlet; - } + return _servlet; } /** @@ -542,9 +505,9 @@ public class ServletHolder extends Holder 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"); } } @@ -553,18 +516,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope */ public boolean isAvailable() { - if (isStarted() && _unavailable == 0) - return true; - try - { - getServlet(); - } - catch (Exception e) - { - LOG.ignore(e); - } - - return isStarted() && _unavailable == 0; + return (isStarted() && !(_servlet instanceof UnavailableServlet)); } /** @@ -575,30 +527,19 @@ public class ServletHolder extends Holder 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) - return; - - _servletHandler.getServletContext().log("unavailable", e); - - _unavailableEx = e; - _unavailable = -1; - if (e.isPermanent()) - _unavailable = -1; - else + synchronized (this) { - if (_unavailableEx.getUnavailableSeconds() > 0) - _unavailable = System.currentTimeMillis() + 1000 * _unavailableEx.getUnavailableSeconds(); - else - _unavailable = System.currentTimeMillis() + 5000; // TODO configure + _servlet = new UnavailableServlet(e, _servlet); + return _servlet; } } @@ -608,37 +549,39 @@ public class ServletHolder extends Holder 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) - { - oldRunAs = _identityService.setRunAs(_identityService.getSystemUserIdentity(), _runAsToken); - } + if (_identityService != null && _runAsToken != null) + _servlet = new RunAsServlet(_servlet, _identityService, _runAsToken); + + if (!isAsyncSupported()) + _servlet = new NotAsyncServlet(_servlet); // Handle configuring servlets that implement org.apache.jasper.servlet.JspServlet if (isJspServlet()) @@ -658,30 +601,21 @@ public class ServletHolder extends Holder implements UserIdentity.Scope catch (UnavailableException e) { makeUnavailable(e); - _servlet = null; - _config = null; - throw e; + if (getServletHandler().isStartWithUnavailable()) + LOG.warn(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); - } } /** @@ -692,7 +626,7 @@ public class ServletHolder extends Holder 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"))) @@ -818,56 +752,23 @@ public class ServletHolder extends Holder 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 - if (_forcedPath != null) - adaptForcedPathToJspContainer(request); - - // 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); + Servlet servlet = getServlet(); + if (servlet == null) + throw new UnavailableException("Servlet Not Initialized"); + servlet.service(request, response); } catch (UnavailableException e) { - makeUnavailable(e); - throw _unavailableEx; - } - finally - { - // Pop run-as role. - if (_identityService != null) - _identityService.unsetRunAs(oldRunAs); + makeUnavailable(e).service(request, response); } } protected boolean isJspServlet() { Servlet servlet = getServletInstance(); - Class c = servlet == null ? _class : servlet.getClass(); + Class c = servlet == null ? getHeldClass() : servlet.getClass(); while (c != null) { @@ -885,11 +786,6 @@ public class ServletHolder extends Holder 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) @@ -1057,7 +953,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope Set 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 @@ -1078,7 +974,7 @@ public class ServletHolder extends Holder 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(); } @@ -1086,7 +982,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope @Override public Collection getMappings() { - ServletMapping[] mappings = _servletHandler.getServletMappings(); + ServletMapping[] mappings = getServletHandler().getServletMappings(); List patterns = new ArrayList(); if (mappings != null) { @@ -1140,7 +1036,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope @Override public Set setServletSecurity(ServletSecurityElement securityElement) { - return _servletHandler.setServletSecurity(this, securityElement); + return getServletHandler().setServletSecurity(this, securityElement); } } @@ -1287,18 +1183,221 @@ public class ServletHolder extends Holder 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); + } + } } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java index a82e7a63452..5e589464a05 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java +++ b/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.addFilter(SingleDispatchFilter.class, "/*", EnumSet.allOf(DispatcherType.class)); - - context.addServlet(DefaultServlet.class, "/"); - context.addServlet(FailServlet.class, "/fail/*"); - context.addServlet(FailClosedServlet.class, "/fail-closed/*"); - context.addServlet(ErrorServlet.class, "/error/*"); - context.addServlet(AppServlet.class, "/app/*"); - context.addServlet(LongerAppServlet.class, "/longer.app/*"); - context.addServlet(SyncSendErrorServlet.class, "/sync/*"); - context.addServlet(AsyncSendErrorServlet.class, "/async/*"); - context.addServlet(NotEnoughServlet.class, "/notenough/*"); - context.addServlet(DeleteServlet.class, "/delete/*"); - context.addServlet(ErrorAndStatusServlet.class, "/error-and-status/*"); + _context.setContextPath("/"); + _context.addFilter(SingleDispatchFilter.class, "/*", EnumSet.allOf(DispatcherType.class)); + _context.addServlet(DefaultServlet.class, "/"); + _context.addServlet(FailServlet.class, "/fail/*"); + _context.addServlet(FailClosedServlet.class, "/fail-closed/*"); + _context.addServlet(ErrorServlet.class, "/error/*"); + _context.addServlet(AppServlet.class, "/app/*"); + _context.addServlet(LongerAppServlet.class, "/longer.app/*"); + _context.addServlet(SyncSendErrorServlet.class, "/sync/*"); + _context.addServlet(AsyncSendErrorServlet.class, "/async/*"); + _context.addServlet(NotEnoughServlet.class, "/notenough/*"); + _context.addServlet(UnavailableServlet.class, "/unavailable/*"); + _context.addServlet(DeleteServlet.class, "/delete/*"); + _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,45 @@ 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 +661,35 @@ 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 dispatches = new ConcurrentHashMap<>(); @@ -665,7 +737,6 @@ public class ErrorPageTest @Override public void destroy() { - } } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/HolderTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/HolderTest.java deleted file mode 100644 index d29eb5baa67..00000000000 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/HolderTest.java +++ /dev/null @@ -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 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 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"); - } -} diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java index f9553980962..c2c5b43dc6e 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java +++ b/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 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 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)); - - assertEquals(null, h.getClassNameForJsp("")); - - assertEquals(null, h.getClassNameForJsp("/blah/")); - - assertEquals(null, h.getClassNameForJsp("//blah///")); - - assertEquals(null, h.getClassNameForJsp("/a/b/c/blah/")); - + assertNull(h.getClassNameForJsp(null)); + assertNull(h.getClassNameForJsp("")); + assertNull(h.getClassNameForJsp("/blah/")); + assertNull(h.getClassNameForJsp("//blah///")); + assertNull(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")); } From ae0ab46066117284fdcffff6daf26c629307f1a7 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 13 Sep 2019 11:40:11 +1000 Subject: [PATCH 2/2] Issue #97 Unavailable handling Added test for servlet component lifecycle Fixed bug where we were destroying listeners before calling them. Signed-off-by: Greg Wilkins --- .../jetty/server/handler/ContextHandler.java | 14 +- .../jetty/servlet/ServletLifeCycleTest.java | 225 ++++++++++++++++++ 2 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 4297a04a031..884669780f3 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -893,9 +893,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); @@ -911,6 +908,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(); } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java new file mode 100644 index 00000000000..f813bcefb4e --- /dev/null +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java @@ -0,0 +1,225 @@ +// +// ======================================================================== +// 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 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 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 + { + } +}