Skip to content

Commit

Permalink
Fixes #55498 ServletHolder cleanup
Browse files Browse the repository at this point in the history 
Various cleanups 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`
  • Loading branch information
@gregw
gregw committed Oct 26, 2020
1 parent 3ec5c34 commit 2047312
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 50 deletions.
14 changes: 3 additions & 11 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java
View file
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -504,19 +498,17 @@ 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;

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;
}
Expand Down
1 change: 1 addition & 0 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java
View file
Expand Up @@ -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);
Expand Down
91 changes: 52 additions & 39 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java
View file
Expand Up @@ -394,12 +394,6 @@ public void doStart()
checkInitOnStartup();

_config = new Config();

synchronized (this)
{
if (getHeldClass() != null && javax.servlet.SingleThreadModel.class.isAssignableFrom(getHeldClass()))
_servlet = new SingleThreadedWrapper();
}
}

@Override
Expand Down Expand Up @@ -465,16 +459,21 @@ 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;
}

/**
Expand Down Expand Up @@ -559,10 +558,17 @@ private synchronized void initServlet()
{
try
{
if (_servlet == null)
_servlet = getInstance();
if (_servlet == null)
_servlet = newInstance();
Servlet servlet = _servlet;
if (servlet == null)
servlet = getInstance();
if (servlet == null)
servlet = newInstance();
if (servlet instanceof javax.servlet.SingleThreadModel)
{
destroyInstance(servlet);
servlet = new SingleThreadedWrapper();
}

if (_config == null)
_config = new Config();

Expand All @@ -578,12 +584,12 @@ private synchronized void initServlet()
if (_identityService != null)
{
_runAsToken = _identityService.newRunAsToken(_runAsRole);
_servlet = new RunAs(_servlet, _identityService, _runAsToken);
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())
Expand All @@ -595,11 +601,12 @@ 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);
servlet.init(_config);
_servlet = servlet;
}
catch (UnavailableException e)
{
Expand Down Expand Up @@ -725,10 +732,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
Expand Down Expand Up @@ -757,7 +770,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);
Expand Down Expand Up @@ -1203,7 +1216,7 @@ public String toString()
private class UnavailableServlet extends GenericServlet
{
final UnavailableException _unavailableException;
final Servlet _servlet;
final Servlet _unavailableServlet;
final long _available;

public UnavailableServlet(UnavailableException unavailableException, Servlet servlet)
Expand All @@ -1212,7 +1225,7 @@ public UnavailableServlet(UnavailableException unavailableException, Servlet ser

if (unavailableException.isPermanent())
{
_servlet = null;
_unavailableServlet = null;
_available = -1;
if (servlet != null)
{
Expand All @@ -1229,7 +1242,7 @@ public UnavailableServlet(UnavailableException unavailableException, Servlet ser
}
else
{
_servlet = servlet;
_unavailableServlet = servlet;
_available = System.nanoTime() + TimeUnit.SECONDS.toNanos(unavailableException.getUnavailableSeconds());
}
}
Expand All @@ -1245,20 +1258,20 @@ else if (System.nanoTime() < _available)
{
synchronized (ServletHolder.this)
{
ServletHolder.this._servlet = this._servlet;
_servlet.service(req, res);
ServletHolder.this._servlet = this._unavailableServlet;
_unavailableServlet.service(req, res);
}
}
}

@Override
public void destroy()
{
if (_servlet != null)
if (_unavailableServlet != null)
{
try
{
destroyInstance(_servlet);
destroyInstance(_unavailableServlet);
}
catch (Throwable th)
{
Expand Down Expand Up @@ -1294,53 +1307,53 @@ public interface WrapFunction

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());
}
}

Expand Down

0 comments on commit 2047312

Please sign in to comment.