Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #5498 ServletHolder cleanup #5514

Merged
merged 8 commits into from
Oct 28, 2020
Merged

Fixes #5498 ServletHolder cleanup #5514

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2047312
Fixes #55498 ServletHolder cleanup
gregw Oct 26, 2020
b1c26d2
Fixes #5498 ServletHolder cleanup
gregw Oct 26, 2020
5efa97b
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-54…
gregw Oct 26, 2020
58afa75
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-54…
gregw Oct 27, 2020
8bc3ebc
Updates from review:
gregw Oct 27, 2020
546fc57
Updates from review:
gregw Oct 27, 2020
c1649d4
Updates from review:
gregw Oct 28, 2020
ea6126a
Updates from review:
gregw Oct 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
123 changes: 75 additions & 48 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -394,12 +395,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 +460,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 @@ -557,12 +557,19 @@ private void makeUnavailable(final Throwable e)
private synchronized void initServlet()
throws ServletException
{
Servlet servlet = _servlet;
try
{
if (_servlet == null)
_servlet = getInstance();
if (_servlet == null)
_servlet = newInstance();
if (servlet == null || servlet instanceof UnavailableServlet)
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 +585,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,14 +602,16 @@ 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)
{
destroyInstance(servlet);
makeUnavailable(e);
if (getServletHandler().isStartWithUnavailable())
LOG.warn(e);
Expand Down Expand Up @@ -725,10 +734,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 +772,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,17 +1218,17 @@ public String toString()
private class UnavailableServlet extends GenericServlet
{
final UnavailableException _unavailableException;
final Servlet _servlet;
final long _available;
final Servlet _unavailableServlet;
final AtomicLong _available = new AtomicLong();

public UnavailableServlet(UnavailableException unavailableException, Servlet servlet)
{
_unavailableException = unavailableException;

if (unavailableException.isPermanent())
{
_servlet = null;
_available = -1;
_unavailableServlet = null;
_available.set(-1);
if (servlet != null)
{
try
Expand All @@ -1229,36 +1244,48 @@ public UnavailableServlet(UnavailableException unavailableException, Servlet ser
}
else
{
_servlet = servlet;
_available = System.nanoTime() + TimeUnit.SECONDS.toNanos(unavailableException.getUnavailableSeconds());
_unavailableServlet = servlet;
_available.set(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
while (true)
{
synchronized (ServletHolder.this)
long available = _available.get();
if (available < 0)
{
((HttpServletResponse)res).sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}

if (available == 0 || System.nanoTime() < available)
{
ServletHolder.this._servlet = this._servlet;
_servlet.service(req, res);
((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
return;
}

if (_available.compareAndSet(available, 0))
{
initServlet();
Request baseRequest = Request.getBaseRequest(req);
ServletHolder.this.prepare(baseRequest, req, res);
ServletHolder.this.handle(baseRequest, req, res);
return;
}
}
}

@Override
public void destroy()
{
if (_servlet != null)
if (_unavailableServlet != null)
{
try
{
destroyInstance(_servlet);
destroyInstance(_unavailableServlet);
}
catch (Throwable th)
{
Expand Down Expand Up @@ -1294,53 +1321,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