Skip to content

Commit

Permalink
Fixes #5498 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`
 + Found and fixed race with making unavaiable servlet available again
  • Loading branch information
gregw committed Oct 26, 2020
1 parent 2047312 commit b1c26d2
Showing 1 changed file with 27 additions and 13 deletions.
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 @@ -556,10 +557,10 @@ private void makeUnavailable(final Throwable e)
private synchronized void initServlet()
throws ServletException
{
Servlet servlet = _servlet;
try
{
Servlet servlet = _servlet;
if (servlet == null)
if (servlet == null || servlet instanceof UnavailableServlet)
servlet = getInstance();
if (servlet == null)
servlet = newInstance();
Expand Down Expand Up @@ -610,6 +611,7 @@ else if (_forcedPath != null)
}
catch (UnavailableException e)
{
destroyInstance(servlet);
makeUnavailable(e);
if (getServletHandler().isStartWithUnavailable())
LOG.warn(e);
Expand Down Expand Up @@ -1217,7 +1219,7 @@ private class UnavailableServlet extends GenericServlet
{
final UnavailableException _unavailableException;
final Servlet _unavailableServlet;
final long _available;
final AtomicLong _available = new AtomicLong();

public UnavailableServlet(UnavailableException unavailableException, Servlet servlet)
{
Expand All @@ -1226,7 +1228,7 @@ public UnavailableServlet(UnavailableException unavailableException, Servlet ser
if (unavailableException.isPermanent())
{
_unavailableServlet = null;
_available = -1;
_available.set(-1);
if (servlet != null)
{
try
Expand All @@ -1243,23 +1245,35 @@ public UnavailableServlet(UnavailableException unavailableException, Servlet ser
else
{
_unavailableServlet = servlet;
_available = System.nanoTime() + TimeUnit.SECONDS.toNanos(unavailableException.getUnavailableSeconds());
_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)
{
((HttpServletResponse)res).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
return;
}

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

0 comments on commit b1c26d2

Please sign in to comment.