Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Greg Wilkins 2020-10-28 17:46:49 +01:00 committed by GitHub
parent b3186a90c2
commit 7a0de6af3a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 142 additions and 121 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java
View File

@ -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 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);
@ -504,9 +498,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;
@ -514,9 +508,7 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory, Welc
if ((_welcomeServlets || _welcomeExactServlets) && welcomeServlet == null)
{
MappedResource<ServletHolder> 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;
}

3
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java
View File

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

1
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java
View File

@ -231,6 +231,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);

2
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ListenerHolder.java
View File

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

227
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java
View File

@ -34,6 +34,7 @@ import java.util.Objects;
import java.util.Set;
import java.util.Stack;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
import javax.servlet.GenericServlet;
import javax.servlet.MultipartConfigElement;
import javax.servlet.Servlet;
@ -80,8 +81,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
private Map<String, String> _roleMap;
private String _forcedPath;
private String _runAsRole;
private RunAsToken _runAsToken;
private IdentityService _identityService;
private ServletRegistration.Dynamic _registration;
private JspContainer _jspContainer;
@ -394,12 +393,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
checkInitOnStartup();
_config = new Config();
synchronized (this)
{
if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass()))
_servlet = new SingleThreadedWrapper();
}
}
@Override
@ -447,13 +440,22 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
Servlet servlet = (Servlet)o;
// need to use the unwrapped servlet because lifecycle callbacks such as
// call any predestroy callbacks
predestroyServlet(servlet);
// Call the servlet destroy
servlet.destroy();
}
private void predestroyServlet(Servlet servlet)
{
// TODO We should only predestroy instnaces that we created
// TODO But this breaks tests in jetty-9, so review behaviour in jetty-10
// Need to use the unwrapped servlet because lifecycle callbacks such as
// postconstruct and predestroy are based off the classname and the wrapper
// classes are unknown outside the ServletHolder
getServletHandler().destroyServlet(unwrap(servlet));
// destroy the wrapped servlet, in case there is special behaviour
servlet.destroy();
}
/**
@ -465,16 +467,20 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
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 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
{
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 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
}
}
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 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
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 @@ public class ServletHolder extends Holder<Servlet> 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");
}
/**
@ -725,10 +747,16 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
protected void prepare(Request baseRequest, ServletRequest request, ServletResponse response)
throws ServletException, UnavailableException
{
// Ensure the servlet is initialized prior to any filters being invoked
getServlet();
MultipartConfigElement mpce = ((Registration)getRegistration()).getMultipartConfig();
if (mpce != null)
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 class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
{
try
{
Servlet servlet = getServlet();
Servlet servlet = getServletInstance();
if (servlet == null)
throw new UnavailableException("Servlet Not Initialized");
servlet.service(request, response);
@ -1197,72 +1225,69 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
@Override
public String toString()
{
return String.format("%s@%x==%s,jsp=%s,order=%d,inst=%b,async=%b", getName(), hashCode(), getClassName(), _forcedPath, _initOrder, _servlet != null, isAsyncSupported());
return String.format("%s==%s@%x{jsp=%s,order=%d,inst=%b,async=%b,src=%s}",
getName(), getClassName(), hashCode(),
_forcedPath, _initOrder, _servlet != null, isAsyncSupported(), getSource());
}
private class UnavailableServlet extends GenericServlet
private class UnavailableServlet extends Wrapper
{
final UnavailableException _unavailableException;
final Servlet _servlet;
final 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
{
synchronized (ServletHolder.this)
{
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(th);
synchronized (this)
{
_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);
}
}
}
@ -1294,53 +1319,53 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
public static class Wrapper implements Servlet, Wrapped<Servlet>
{
private final Servlet _servlet;
private final Servlet _wrappedServlet;
public Wrapper(Servlet servlet)
{
_servlet = Objects.requireNonNull(servlet, "Servlet cannot be null");
_wrappedServlet = Objects.requireNonNull(servlet, "Servlet cannot be null");
}
@Override
public Servlet getWrapped()
{
return _servlet;
return _wrappedServlet;
}
@Override
public void init(ServletConfig config) throws ServletException
{
_servlet.init(config);
_wrappedServlet.init(config);
}
@Override
public ServletConfig getServletConfig()
{
return _servlet.getServletConfig();
return _wrappedServlet.getServletConfig();
}
@Override
public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException
{
_servlet.service(req, res);
_wrappedServlet.service(req, res);
}
@Override
public String getServletInfo()
{
return _servlet.getServletInfo();
return _wrappedServlet.getServletInfo();
}
@Override
public void destroy()
{
_servlet.destroy();
_wrappedServlet.destroy();
}
@Override
public String toString()
{
return String.format("%s:%s", this.getClass().getSimpleName(), _servlet.toString());
return String.format("%s:%s", this.getClass().getSimpleName(), _wrappedServlet.toString());
}
}

1
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java
View File

@ -422,6 +422,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());
}
}

4
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java
View File

@ -59,6 +59,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!

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

@ -20,6 +20,7 @@ package org.eclipse.jetty.util.component;
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 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());
}
}