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 e7cb6316a9d..dbeae7ccc0c 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 @@ -68,6 +68,11 @@ public abstract class BaseHolder extends AbstractLifeCycle implements Dumpabl return _lock.lock(); } + boolean lockIsHeldByCurrentThread() + { + return _lock.isHeldByCurrentThread(); + } + /** * Do any setup necessary after starting * diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java index e704b6f0646..40f6bbdba7d 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java @@ -148,7 +148,6 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc private boolean _useFileMappedBuffer = false; private String _relativeResourceBase; private ServletHandler _servletHandler; - private ServletHolder _defaultHolder; public DefaultServlet(ResourceService resourceService) { @@ -304,11 +303,6 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc _resourceService.setGzipEquivalentFileExtensions(gzipEquivalentFileExtensions); _servletHandler = _contextHandler.getChildHandlerByClass(ServletHandler.class); - for (ServletHolder h : _servletHandler.getServlets()) - { - if (h.getServletInstance() == this) - _defaultHolder = h; - } if (LOG.isDebugEnabled()) LOG.debug("resource base = {}", _resourceBase); @@ -499,9 +493,9 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc return null; String welcomeServlet = null; - for (int i = 0; i < _welcomes.length; i++) + for (String s : _welcomes) { - String welcomeInContext = URIUtil.addPaths(pathInContext, _welcomes[i]); + String welcomeInContext = URIUtil.addPaths(pathInContext, s); Resource welcome = getResource(welcomeInContext); if (welcome != null && welcome.exists()) return welcomeInContext; @@ -509,9 +503,7 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc if ((_welcomeServlets || _welcomeExactServlets) && welcomeServlet == null) { ServletHandler.MappedServlet entry = _servletHandler.getMappedServlet(welcomeInContext); - @SuppressWarnings("ReferenceEquality") - boolean isDefaultHolder = (entry.getServletHolder() != _defaultHolder); - if (entry != null && isDefaultHolder && + if (entry != null && entry.getServletHolder().getServletInstance() != this && (_welcomeServlets || (_welcomeExactServlets && entry.getPathSpec().getDeclaration().equals(welcomeInContext)))) welcomeServlet = welcomeInContext; } 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 579ae8dddb2..4613bed7389 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 @@ -236,7 +236,8 @@ public class FilterHolder extends Holder @Override public String toString() { - return String.format("%s@%x==%s,inst=%b,async=%b", getName(), hashCode(), getClassName(), _filter != null, isAsyncSupported()); + return String.format("%s==%s@%x{inst=%b,async=%b,src=%s}", + getName(), getClassName(), hashCode(), _filter != null, isAsyncSupported(), getSource()); } public FilterRegistration.Dynamic getRegistration() diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java index d5d950bdf20..c221ff0f3ab 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java @@ -229,6 +229,7 @@ public class Invoker extends HttpServlet if (holder != null) { final Request baseRequest = Request.getBaseRequest(request); + holder.prepare(baseRequest, request, response); holder.handle(baseRequest, new InvokedRequest(request, included, servlet, servletPath, pathInfo), response); 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 14aafaf9107..c129c9348b9 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 @@ -137,7 +137,7 @@ public class ListenerHolder extends BaseHolder @Override public String toString() { - return super.toString() + ": " + getClassName(); + return String.format("%s@%x{src=%s}", getClassName(), hashCode(), getSource()); } /** 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 49430e94f04..3baa3e88245 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 @@ -34,6 +34,7 @@ import java.util.Objects; import java.util.Set; import java.util.Stack; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; import javax.servlet.GenericServlet; import javax.servlet.MultipartConfigElement; import javax.servlet.Servlet; @@ -80,8 +81,6 @@ public class ServletHolder extends Holder implements UserIdentity.Scope private Map _roleMap; private String _forcedPath; private String _runAsRole; - private RunAsToken _runAsToken; - private IdentityService _identityService; private ServletRegistration.Dynamic _registration; private JspContainer _jspContainer; @@ -399,12 +398,6 @@ public class ServletHolder extends Holder implements UserIdentity.Scope checkInitOnStartup(); _config = new Config(); - - try (AutoLock l = lock()) - { - if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass())) - _servlet = new SingleThreadedWrapper(); - } } @Override @@ -452,13 +445,22 @@ public class ServletHolder extends Holder implements UserIdentity.Scope Servlet servlet = (Servlet)o; - // need to use the unwrapped servlet because lifecycle callbacks such as + // call any predestroy callbacks + predestroyServlet(servlet); + + // Call the servlet destroy + servlet.destroy(); + } + + private void predestroyServlet(Servlet servlet) + { + // TODO We should only predestroy instnaces that we created + // TODO But this breaks tests in jetty-9, so review behaviour in jetty-10 + + // Need to use the unwrapped servlet because lifecycle callbacks such as // postconstruct and predestroy are based off the classname and the wrapper // classes are unknown outside the ServletHolder getServletHandler().destroyServlet(unwrap(servlet)); - - // destroy the wrapped servlet, in case there is special behaviour - servlet.destroy(); } /** @@ -470,16 +472,20 @@ public class ServletHolder extends Holder implements UserIdentity.Scope public Servlet getServlet() throws ServletException { - try (AutoLock l = lock()) + Servlet servlet = _servlet; + if (servlet == null) { - if (_servlet == null && isRunning()) + try (AutoLock l = lock()) { - if (getHeldClass() != null) - initServlet(); + if (_servlet == null && isRunning()) + { + if (getHeldClass() != null) + initServlet(); + } + servlet = _servlet; } } - - return _servlet; + return servlet; } /** @@ -533,7 +539,16 @@ public class ServletHolder extends Holder implements UserIdentity.Scope { try (AutoLock l = lock()) { - _servlet = new UnavailableServlet(e, _servlet); + if (_servlet instanceof UnavailableServlet) + { + Throwable cause = ((UnavailableServlet)_servlet).getUnavailableException(); + if (cause != e) + cause.addSuppressed(e); + } + else + { + _servlet = new UnavailableServlet(e, _servlet); + } return _servlet; } } @@ -562,33 +577,40 @@ public class ServletHolder extends Holder implements UserIdentity.Scope private void initServlet() throws ServletException { - try (AutoLock l = lock()) + // must be called with lock held and _servlet==null + if (!lockIsHeldByCurrentThread()) + throw new IllegalStateException("Lock not held"); + if (_servlet != null) + throw new IllegalStateException("Servlet already initialised: " + _servlet); + + Servlet servlet = null; + try { - if (_servlet == null) - _servlet = getInstance(); - if (_servlet == null) - _servlet = newInstance(); + servlet = getInstance(); + if (servlet == null) + servlet = newInstance(); + if (servlet instanceof javax.servlet.SingleThreadModel) + { + predestroyServlet(servlet); + servlet = new SingleThreadedWrapper(); + } + if (_config == null) _config = new Config(); //check run-as rolename and convert to token from IdentityService - if (_runAsRole == null) + if (_runAsRole != null) { - _identityService = null; - _runAsToken = null; - } - else - { - _identityService = getServletHandler().getIdentityService(); - if (_identityService != null) + IdentityService identityService = getServletHandler().getIdentityService(); + if (identityService != null) { - _runAsToken = _identityService.newRunAsToken(_runAsRole); - _servlet = new RunAs(_servlet, _identityService, _runAsToken); + RunAsToken runAsToken = identityService.newRunAsToken(_runAsRole); + servlet = new RunAs(servlet, identityService, runAsToken); } } if (!isAsyncSupported()) - _servlet = new NotAsync(_servlet); + servlet = new NotAsync(servlet); // Handle configuring servlets that implement org.apache.jasper.servlet.JspServlet if (isJspServlet()) @@ -599,28 +621,30 @@ public class ServletHolder extends Holder implements UserIdentity.Scope else if (_forcedPath != null) detectJspContainer(); - _servlet = wrap(_servlet, WrapFunction.class, WrapFunction::wrapServlet); + servlet = wrap(servlet, WrapFunction.class, WrapFunction::wrapServlet); if (LOG.isDebugEnabled()) LOG.debug("Servlet.init {} for {}", _servlet, getName()); - _servlet.init(_config); - } - catch (UnavailableException e) - { - makeUnavailable(e); - if (getServletHandler().isStartWithUnavailable()) - LOG.warn("{} is marked as Unavailable", this, e); - else - throw e; + try + { + servlet.init(_config); + _servlet = servlet; + } + catch (UnavailableException e) + { + _servlet = new UnavailableServlet(e, servlet); + } } catch (ServletException e) { makeUnavailable(e.getCause() == null ? e : e.getCause()); + predestroyServlet(servlet); throw e; } catch (Exception e) { makeUnavailable(e); + predestroyServlet(servlet); throw new ServletException(this.toString(), e); } } @@ -655,8 +679,8 @@ public class ServletHolder extends Holder implements UserIdentity.Scope } scratch = new File(getInitParameter("scratchdir")); - if (!scratch.exists()) - scratch.mkdir(); + if (!scratch.exists() && !scratch.mkdir()) + throw new IllegalStateException("Could not create JSP scratch directory"); } @Override @@ -700,10 +724,16 @@ public class ServletHolder extends Holder implements UserIdentity.Scope protected void prepare(Request baseRequest, ServletRequest request, ServletResponse response) throws ServletException, UnavailableException { + // Ensure the servlet is initialized prior to any filters being invoked getServlet(); - MultipartConfigElement mpce = ((Registration)getRegistration()).getMultipartConfig(); - if (mpce != null) - baseRequest.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); + + // Check for multipart config + if (_registration != null) + { + MultipartConfigElement mpce = ((Registration)_registration).getMultipartConfig(); + if (mpce != null) + baseRequest.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); + } } /** @@ -725,7 +755,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope { try { - Servlet servlet = getServlet(); + Servlet servlet = getServletInstance(); if (servlet == null) throw new UnavailableException("Servlet Not Initialized"); servlet.service(request, response); @@ -1160,72 +1190,69 @@ public class ServletHolder extends Holder implements UserIdentity.Scope @Override public String toString() { - return String.format("%s@%x==%s,jsp=%s,order=%d,inst=%b,async=%b", getName(), hashCode(), getClassName(), _forcedPath, _initOrder, _servlet != null, isAsyncSupported()); + return String.format("%s==%s@%x{jsp=%s,order=%d,inst=%b,async=%b,src=%s}", + getName(), getClassName(), hashCode(), + _forcedPath, _initOrder, _servlet != null, isAsyncSupported(), getSource()); } - private class UnavailableServlet extends GenericServlet + private class UnavailableServlet extends Wrapper { final UnavailableException _unavailableException; - final Servlet _servlet; - final long _available; + final AtomicLong _unavailableStart; public UnavailableServlet(UnavailableException unavailableException, Servlet servlet) { + super(servlet != null ? servlet : new GenericServlet() + { + @Override + public void service(ServletRequest req, ServletResponse res) throws IOException + { + ((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND); + } + }); _unavailableException = unavailableException; if (unavailableException.isPermanent()) - { - _servlet = null; - _available = -1; - if (servlet != null) - { - try - { - destroyInstance(servlet); - } - catch (Throwable th) - { - if (th != unavailableException) - unavailableException.addSuppressed(th); - } - } - } + _unavailableStart = null; else { - _servlet = servlet; - _available = System.nanoTime() + TimeUnit.SECONDS.toNanos(unavailableException.getUnavailableSeconds()); + long start = System.nanoTime(); + while (start == 0) + start = System.nanoTime(); + _unavailableStart = new AtomicLong(start); } } @Override public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException { - if (_available == -1) + if (LOG.isDebugEnabled()) + LOG.debug("Unavailable {}", req, _unavailableException); + if (_unavailableStart == null) + { ((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND); - else if (System.nanoTime() < _available) - ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); + } else { - try (AutoLock l = lock()) - { - ServletHolder.this._servlet = this._servlet; - _servlet.service(req, res); - } - } - } + long start = _unavailableStart.get(); - @Override - public void destroy() - { - if (_servlet != null) - { - try + if (start == 0 || System.nanoTime() - start < TimeUnit.SECONDS.toNanos(_unavailableException.getUnavailableSeconds())) { - destroyInstance(_servlet); + ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); } - catch (Throwable th) + else if (_unavailableStart.compareAndSet(start, 0)) { - LOG.warn("Unable to destroy {}", _servlet, th); + try (AutoLock l = lock()) + { + _servlet = getWrapped(); + } + Request baseRequest = Request.getBaseRequest(req); + ServletHolder.this.prepare(baseRequest, req, res); + ServletHolder.this.handle(baseRequest, req, res); + } + else + { + ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); } } } @@ -1257,53 +1284,53 @@ public class ServletHolder extends Holder implements UserIdentity.Scope public static class Wrapper implements Servlet, Wrapped { - private final Servlet _servlet; + private final Servlet _wrappedServlet; public Wrapper(Servlet servlet) { - _servlet = Objects.requireNonNull(servlet, "Servlet cannot be null"); + _wrappedServlet = Objects.requireNonNull(servlet, "Servlet cannot be null"); } @Override public Servlet getWrapped() { - return _servlet; + return _wrappedServlet; } @Override public void init(ServletConfig config) throws ServletException { - _servlet.init(config); + _wrappedServlet.init(config); } @Override public ServletConfig getServletConfig() { - return _servlet.getServletConfig(); + return _wrappedServlet.getServletConfig(); } @Override public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException { - _servlet.service(req, res); + _wrappedServlet.service(req, res); } @Override public String getServletInfo() { - return _servlet.getServletInfo(); + return _wrappedServlet.getServletInfo(); } @Override public void destroy() { - _servlet.destroy(); + _wrappedServlet.destroy(); } @Override public String toString() { - return String.format("%s:%s", this.getClass().getSimpleName(), _servlet.toString()); + return String.format("%s:%s", this.getClass().getSimpleName(), _wrappedServlet.toString()); } } 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 ffadf338df1..ad37d08b5d5 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 @@ -459,6 +459,7 @@ public class ErrorPageTest __destroyed = new AtomicBoolean(false); String response = _connector.getResponse("GET /unavailable/info HTTP/1.0\r\n\r\n"); assertThat(response, Matchers.containsString("HTTP/1.1 404 ")); + _server.stop(); assertTrue(__destroyed.get()); } } 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 index 7832100e4e6..9c999d73c29 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java @@ -60,6 +60,10 @@ public class ServletLifeCycleTest context.getObjectFactory().addDecorator(new TestDecorator()); + // TODO review this test in jetty-10. Instances that are created externally and passed in should not be + // TODO decorated by the object factory unless: a) there is an explicit call to ServletContext.createXxx; + // TODO ; and b) the Servlet dyanmic API is used to register them. + ServletHandler sh = context.getServletHandler(); sh.addListener(new ListenerHolder(TestListener.class)); //added directly to ServletHandler context.addEventListener(context.getServletContext().createListener(TestListener2.class));//create,decorate and add listener to context - no holder! diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/component/AbstractLifeCycle.java b/jetty-util/src/main/java/org/eclipse/jetty/util/component/AbstractLifeCycle.java index 5f970cf19cb..ee274f37cc4 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/component/AbstractLifeCycle.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/component/AbstractLifeCycle.java @@ -23,6 +23,7 @@ import java.util.EventListener; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Uptime; import org.eclipse.jetty.util.annotation.ManagedAttribute; import org.eclipse.jetty.util.annotation.ManagedObject; @@ -334,13 +335,9 @@ public abstract class AbstractLifeCycle implements LifeCycle @Override public String toString() { - Class clazz = getClass(); - String name = clazz.getSimpleName(); - if ((name == null || name.length() == 0) && clazz.getSuperclass() != null) - { - clazz = clazz.getSuperclass(); - name = clazz.getSimpleName(); - } + String name = getClass().getSimpleName(); + if (StringUtil.isBlank(name) && getClass().getSuperclass() != null) + name = getClass().getSuperclass().getSimpleName(); return String.format("%s@%x{%s}", name, hashCode(), getState()); }