From 7a0de6af3a41fd6025f55d27c1e12103625347cd Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 28 Oct 2020 17:46:49 +0100 Subject: [PATCH] Fixes #5498 ServletHolder cleanup (#5514) * Fixes #55498 ServletHolder cleanup Various cleanups for #5498 including: + renaming multiple `_servlet` fields in inner classes to avoid confusion + better comments in prepare method to describe why it is needed + call prepare from Invoker servlet + The `_servlet` field is not set until after the servlet is initialized + Consistent wrapping of `SingleThreadedWrapper` now in `initServlet` + The `getServlet` method now looks the volatile `_servlet` to avoid locking if possible + The `handle` method now calls `getServletInstance` as servlet will have been initialized in `prepare` + Found and fixed race with making unavaiable servlet available again + fixed nanotime overflow + fixed several compiler warnings/suggestions + removed while true from unavailable servlet + Do not destroy servlets unless init has been called. + Added TODOs about calling predestroy on instances not created by the holder. + Do not destroy servlets unless init has been called. + Added TODOs about calling predestroy on instances not created by the holder. + improved dump and toString --- .../eclipse/jetty/servlet/DefaultServlet.java | 14 +- .../eclipse/jetty/servlet/FilterHolder.java | 3 +- .../org/eclipse/jetty/servlet/Invoker.java | 1 + .../eclipse/jetty/servlet/ListenerHolder.java | 2 +- .../eclipse/jetty/servlet/ServletHolder.java | 225 ++++++++++-------- .../eclipse/jetty/servlet/ErrorPageTest.java | 1 + .../jetty/servlet/ServletLifeCycleTest.java | 4 + .../util/component/AbstractLifeCycle.java | 11 +- 8 files changed, 141 insertions(+), 120 deletions(-) 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 f242949cf74f..04dcd3e21938 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 @@ -149,7 +149,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) { @@ -303,11 +302,6 @@ public void init() _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); @@ -504,9 +498,9 @@ public String getWelcomeFile(String pathInContext) 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; @@ -514,9 +508,7 @@ public String getWelcomeFile(String pathInContext) if ((_welcomeServlets || _welcomeExactServlets) && welcomeServlet == null) { MappedResource entry = _servletHandler.getMappedServlet(welcomeInContext); - @SuppressWarnings("ReferenceEquality") - boolean isDefaultHolder = (entry.getResource() != _defaultHolder); - if (entry != null && isDefaultHolder && + if (entry != null && entry.getResource().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 6f4af7368b78..320d93eb8e6a 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 @@ -222,7 +222,8 @@ public void dump(Appendable out, String indent) throws IOException @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 09cb5ee81f30..1fe1ef1f7b48 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 @@ -231,6 +231,7 @@ protected void service(HttpServletRequest request, HttpServletResponse response) 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 42d4c0549a82..2b1cb62c7a0c 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 @@ -126,7 +126,7 @@ public void doStop() throws Exception @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 4929c16f9300..f84d21c4a46e 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.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; @@ -394,12 +393,6 @@ public void doStart() checkInitOnStartup(); _config = new Config(); - - synchronized (this) - { - if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass())) - _servlet = new SingleThreadedWrapper(); - } } @Override @@ -447,13 +440,22 @@ public void destroyInstance(Object o) 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(); } /** @@ -465,16 +467,20 @@ public void destroyInstance(Object o) public Servlet getServlet() throws ServletException { - synchronized (this) + Servlet servlet = _servlet; + if (servlet == null) { - if (_servlet == null && isRunning()) + synchronized (this) { - if (getHeldClass() != null) - initServlet(); + if (_servlet == null && isRunning()) + { + if (getHeldClass() != null) + initServlet(); + } + servlet = _servlet; } } - - return _servlet; + return servlet; } /** @@ -528,7 +534,16 @@ private Servlet makeUnavailable(UnavailableException e) { synchronized (this) { - _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; } } @@ -554,36 +569,41 @@ private void makeUnavailable(final Throwable e) } } - private synchronized void initServlet() + private void initServlet() throws ServletException { + // must be called with lock held and _servlet==null + 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()) @@ -595,28 +615,30 @@ else if (_forcedPath != null) detectJspContainer(); initMultiPart(); - _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(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); } } @@ -651,8 +673,8 @@ protected void initJspServlet() throws Exception } scratch = new File(getInitParameter("scratchdir")); - if (!scratch.exists()) - scratch.mkdir(); + if (!scratch.exists() && !scratch.mkdir()) + throw new IllegalStateException("Could not create JSP scratch directory"); } /** @@ -725,10 +747,16 @@ public void setRunAsRole(String role) 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); + } } @Deprecated @@ -757,7 +785,7 @@ public void handle(Request baseRequest, { try { - Servlet servlet = getServlet(); + Servlet servlet = getServletInstance(); if (servlet == null) throw new UnavailableException("Servlet Not Initialized"); servlet.service(request, response); @@ -1197,72 +1225,69 @@ public void dump(Appendable out, String indent) throws IOException @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) { - _unavailableException = unavailableException; - - if (unavailableException.isPermanent()) + super(servlet != null ? servlet : new GenericServlet() { - _servlet = null; - _available = -1; - if (servlet != null) + @Override + public void service(ServletRequest req, ServletResponse res) throws IOException { - try - { - destroyInstance(servlet); - } - catch (Throwable th) - { - if (th != unavailableException) - unavailableException.addSuppressed(th); - } + ((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND); } - } + }); + _unavailableException = unavailableException; + + if (unavailableException.isPermanent()) + _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 { - synchronized (ServletHolder.this) + long start = _unavailableStart.get(); + + if (start == 0 || System.nanoTime() - start < TimeUnit.SECONDS.toNanos(_unavailableException.getUnavailableSeconds())) { - ServletHolder.this._servlet = this._servlet; - _servlet.service(req, res); + ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); } - } - } - - @Override - public void destroy() - { - if (_servlet != null) - { - try + else if (_unavailableStart.compareAndSet(start, 0)) { - destroyInstance(_servlet); + synchronized (this) + { + _servlet = getWrapped(); + } + Request baseRequest = Request.getBaseRequest(req); + ServletHolder.this.prepare(baseRequest, req, res); + ServletHolder.this.handle(baseRequest, req, res); } - catch (Throwable th) + else { - LOG.warn(th); + ((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); } } } @@ -1294,53 +1319,53 @@ public interface WrapFunction 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 45668965ed7f..59064a280a57 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 @@ -422,6 +422,7 @@ public void testPermanentlyUnavailable() throws Exception __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 0e49712f836b..65d92ee5d5a0 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 @@ -59,6 +59,10 @@ public void testLifeCycle() throws Exception 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 3a9b07e795a8..1fb5793ac6fd 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 @@ -20,6 +20,7 @@ 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; @@ -280,13 +281,9 @@ public void lifeCycleStopping(LifeCycle event) @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()); } }