From a46ed5b5f06332e8d55ac4c9ee5d81d0ee219e2e Mon Sep 17 00:00:00 2001
From: Thomas Draebing <thomas.draebing@sap.com>
Date: Wed, 19 Aug 2020 09:57:28 +0200
Subject: [PATCH 1/8] Also manage graceful shutdown of already started async
 requests

Signed-off-by: Thomas Draebing <thomas.draebing@sap.com>
---
 .../org/eclipse/jetty/server/handler/StatisticsHandler.java     | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
index b918a95ab7af..1a3a5a16fa8f 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
@@ -189,7 +189,7 @@ else if (_wrapWarning.compareAndSet(false, true))
             _dispatchedStats.decrement();
             _dispatchedTimeStats.record(dispatched);
 
-            if (state.isSuspended())
+            if (state.isSuspended() || state.isAsyncStarted())
             {
                 if (state.isInitial())
                 {

From 32358b1b7762cda392d499d1cd2077d1ab2b164d Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Wed, 19 Aug 2020 14:34:28 +1000
Subject: [PATCH 2/8] Issue #5105 - fix StatisticsHandler bug with async
 dispatched requests

If the request is async dispatched, the check state.isSuspended() is not
correct to determine if the request was async or not. The check
state.isAsyncStarted() should be used instead.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../server/handler/StatisticsHandler.java     | 76 ++++++++++---------
 .../server/handler/StatisticsHandlerTest.java | 59 ++++++++++++++
 2 files changed, 99 insertions(+), 36 deletions(-)

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
index 1a3a5a16fa8f..4574cd926775 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
@@ -20,7 +20,6 @@
 
 import java.io.IOException;
 import java.util.concurrent.Future;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.LongAdder;
 import javax.servlet.AsyncEvent;
@@ -29,7 +28,6 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.server.AsyncContextEvent;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.HttpChannelState;
@@ -59,6 +57,7 @@ public class StatisticsHandler extends HandlerWrapper implements Graceful
 
     private final LongAdder _asyncDispatches = new LongAdder();
     private final LongAdder _expires = new LongAdder();
+    private final LongAdder _errors = new LongAdder();
 
     private final LongAdder _responses1xx = new LongAdder();
     private final LongAdder _responses2xx = new LongAdder();
@@ -76,30 +75,30 @@ protected FutureCallback newShutdownCallback()
         }
     };
 
-    private final AtomicBoolean _wrapWarning = new AtomicBoolean();
-
     private final AsyncListener _onCompletion = new AsyncListener()
     {
         @Override
-        public void onTimeout(AsyncEvent event) throws IOException
+        public void onStartAsync(AsyncEvent event) throws IOException
         {
-            _expires.increment();
+            event.getAsyncContext().addListener(this);
         }
 
         @Override
-        public void onStartAsync(AsyncEvent event) throws IOException
+        public void onTimeout(AsyncEvent event) throws IOException
         {
-            event.getAsyncContext().addListener(this);
+            _expires.increment();
         }
 
         @Override
         public void onError(AsyncEvent event) throws IOException
         {
+            _errors.increment();
         }
 
         @Override
         public void onComplete(AsyncEvent event) throws IOException
         {
+            System.err.println("On Async Complete for " + event);
             HttpChannelState state = ((AsyncContextEvent)event).getHttpChannelState();
 
             Request request = state.getBaseRequest();
@@ -149,6 +148,10 @@ public void statsReset()
     @Override
     public void handle(String path, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
     {
+        Handler handler = getHandler();
+        if (handler == null || !isStarted() || isShutdown())
+            return;
+
         _dispatchedStats.increment();
 
         final long start;
@@ -168,51 +171,40 @@ public void handle(String path, Request baseRequest, HttpServletRequest request,
 
         try
         {
-            Handler handler = getHandler();
-            if (handler != null && !_shutdown.isShutdown() && isStarted())
-                handler.handle(path, baseRequest, request, response);
-            else
-            {
-                if (!baseRequest.isHandled())
-                    baseRequest.setHandled(true);
-                else if (_wrapWarning.compareAndSet(false, true))
-                    LOG.warn("Bad statistics configuration. Latencies will be incorrect in {}", this);
-                if (!baseRequest.getResponse().isCommitted())
-                    response.sendError(HttpStatus.SERVICE_UNAVAILABLE_503);
-            }
+            handler.handle(path, baseRequest, request, response);
         }
         finally
         {
             final long now = System.currentTimeMillis();
             final long dispatched = now - start;
 
+            // TODO: make dispatchedStats optional metric for shutdown
             _dispatchedStats.decrement();
             _dispatchedTimeStats.record(dispatched);
 
-            if (state.isSuspended() || state.isAsyncStarted())
+            if (state.isInitial())
             {
-                if (state.isInitial())
+                if (state.isAsyncStarted())
                 {
                     state.addListener(_onCompletion);
                     _asyncWaitStats.increment();
                 }
-            }
-            else if (state.isInitial())
-            {
-                long d = _requestStats.decrement();
-                _requestTimeStats.record(dispatched);
-                updateResponse(baseRequest);
-
-                // If we have no more dispatches, should we signal shutdown?
-                FutureCallback shutdown = _shutdown.get();
-                if (shutdown != null)
+                else
                 {
-                    response.flushBuffer();
-                    if (d == 0)
-                        shutdown.succeeded();
+                    long d = _requestStats.decrement();
+                    _requestTimeStats.record(dispatched);
+                    updateResponse(baseRequest);
+
+                    // If we have no more dispatches, should we signal shutdown?
+                    FutureCallback shutdown = _shutdown.get();
+                    if (shutdown != null)
+                    {
+                        response.flushBuffer();
+                        if (d == 0)
+                            shutdown.succeeded();
+                    }
                 }
             }
-            // else onCompletion will handle it.
         }
     }
 
@@ -251,6 +243,8 @@ protected void updateResponse(Request request)
     @Override
     protected void doStart() throws Exception
     {
+        if (getHandler() == null)
+            throw new IllegalStateException("StatisticsHandler has no Wrapped Handler");
         _shutdown.cancel();
         super.doStart();
         statsReset();
@@ -467,6 +461,16 @@ public int getExpires()
         return _expires.intValue();
     }
 
+    /**
+     * @return the number of async errors that occurred.
+     * @see #getAsyncDispatches()
+     */
+    @ManagedAttribute("number of async errors that occurred")
+    public int getErrors()
+    {
+        return _errors.intValue();
+    }
+
     /**
      * @return the number of responses with a 1xx status returned by this context
      * since {@link #statsReset()} was last called.
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
index 8216ec215512..152d1b6c4efd 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
@@ -363,6 +363,65 @@ public void onComplete(AsyncEvent event)
         assertTrue(_statsHandler.getDispatchedTimeMax() + dispatchTime <= _statsHandler.getDispatchedTimeTotal());
     }
 
+    @Test
+    public void asyncDispatchTest() throws Exception
+    {
+        final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
+        final CyclicBarrier[] barrier = {new CyclicBarrier(2), new CyclicBarrier(2), new CyclicBarrier(2), new CyclicBarrier(2)};
+        _statsHandler.setHandler(new AbstractHandler()
+        {
+            @Override
+            public void handle(String path, Request request, HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws ServletException
+            {
+                request.setHandled(true);
+                try
+                {
+                    if (asyncHolder.get() == null)
+                    {
+                        barrier[0].await();
+                        barrier[1].await();
+                        AsyncContext asyncContext = request.startAsync();
+                        asyncHolder.set(asyncContext);
+                        asyncContext.dispatch();
+                    }
+                    else
+                    {
+                        barrier[2].await();
+                        barrier[3].await();
+                    }
+                }
+                catch (Exception x)
+                {
+                    throw new ServletException(x);
+                }
+            }
+        });
+        _server.start();
+
+        String request = "GET / HTTP/1.1\r\n" +
+            "Host: localhost\r\n" +
+            "\r\n";
+        _connector.executeRequest(request);
+
+        // Before we have started async we have one active request.
+        barrier[0].await();
+        assertEquals(1, _statistics.getConnections());
+        assertEquals(1, _statsHandler.getRequests());
+        assertEquals(1, _statsHandler.getRequestsActive());
+        assertEquals(1, _statsHandler.getDispatched());
+        assertEquals(1, _statsHandler.getDispatchedActive());
+        barrier[1].await();
+
+        // After we are async the same request should still be active even though we have async dispatched.
+        barrier[2].await();
+        assertEquals(1, _statistics.getConnections());
+        assertEquals(1, _statsHandler.getRequests());
+        assertEquals(1, _statsHandler.getRequestsActive());
+        assertEquals(2, _statsHandler.getDispatched());
+        assertEquals(1, _statsHandler.getDispatchedActive());
+        barrier[3].await();
+    }
+
     @Test
     public void testSuspendExpire() throws Exception
     {

From a65f00156d7f467c244507127411cb77a286139e Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Wed, 19 Aug 2020 18:30:47 +1000
Subject: [PATCH 3/8] Issue #5105 - add optional configuration to not wait for
 suspended requests in StatisticsHandler

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../src/main/config/etc/jetty-stats.xml       |   6 +-
 .../src/main/config/modules/stats.mod         |   5 +
 .../server/handler/StatisticsHandler.java     |  42 ++++---
 .../server/handler/StatisticsHandlerTest.java | 111 ++++++++++++++++++
 4 files changed, 148 insertions(+), 16 deletions(-)

diff --git a/jetty-server/src/main/config/etc/jetty-stats.xml b/jetty-server/src/main/config/etc/jetty-stats.xml
index bc8a649c81a3..9423fad2abd4 100644
--- a/jetty-server/src/main/config/etc/jetty-stats.xml
+++ b/jetty-server/src/main/config/etc/jetty-stats.xml
@@ -5,7 +5,11 @@
 <Configure id="Server" class="org.eclipse.jetty.server.Server">
   <Call name="insertHandler">
     <Arg>
-      <New id="StatsHandler" class="org.eclipse.jetty.server.handler.StatisticsHandler"></New>
+      <New id="StatsHandler" class="org.eclipse.jetty.server.handler.StatisticsHandler">
+        <Set name="waitForSuspendedRequestsOnShutdown">
+          <Property name="jetty.statistics.waitForSuspendedRequestsOnShutdown" default="true"/>
+        </Set>
+      </New>
     </Arg>
   </Call>
   <Call class="org.eclipse.jetty.server.ServerConnectionStatistics" name="addToAllConnectors">
diff --git a/jetty-server/src/main/config/modules/stats.mod b/jetty-server/src/main/config/modules/stats.mod
index 8bf06451c8fa..be551a058d01 100644
--- a/jetty-server/src/main/config/modules/stats.mod
+++ b/jetty-server/src/main/config/modules/stats.mod
@@ -15,3 +15,8 @@ etc/jetty-stats.xml
 
 [ini]
 jetty.webapp.addServerClasses+=,-org.eclipse.jetty.servlet.StatisticsServlet
+
+[ini-template]
+
+## If the Graceful shutdown should wait for suspended requests as well as dispatched ones.
+# jetty.statistics.waitForSuspendedRequestsOnShutdown=true
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
index 4574cd926775..059541fa17a7 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
@@ -66,6 +66,8 @@ public class StatisticsHandler extends HandlerWrapper implements Graceful
     private final LongAdder _responses5xx = new LongAdder();
     private final LongAdder _responsesTotalBytes = new LongAdder();
 
+    private boolean waitForSuspendedRequestsOnShutdown = true;
+
     private final Graceful.Shutdown _shutdown = new Graceful.Shutdown()
     {
         @Override
@@ -98,13 +100,12 @@ public void onError(AsyncEvent event) throws IOException
         @Override
         public void onComplete(AsyncEvent event) throws IOException
         {
-            System.err.println("On Async Complete for " + event);
             HttpChannelState state = ((AsyncContextEvent)event).getHttpChannelState();
 
             Request request = state.getBaseRequest();
             final long elapsed = System.currentTimeMillis() - request.getTimeStamp();
 
-            long d = _requestStats.decrement();
+            long numRequests = _requestStats.decrement();
             _requestTimeStats.record(elapsed);
 
             updateResponse(request);
@@ -112,7 +113,7 @@ public void onComplete(AsyncEvent event) throws IOException
             _asyncWaitStats.decrement();
 
             // If we have no more dispatches, should we signal shutdown?
-            if (d == 0)
+            if (numRequests == 0 && waitForSuspendedRequestsOnShutdown)
             {
                 FutureCallback shutdown = _shutdown.get();
                 if (shutdown != null)
@@ -178,8 +179,8 @@ public void handle(String path, Request baseRequest, HttpServletRequest request,
             final long now = System.currentTimeMillis();
             final long dispatched = now - start;
 
-            // TODO: make dispatchedStats optional metric for shutdown
-            _dispatchedStats.decrement();
+            long numRequests = -1;
+            long numDispatches = _dispatchedStats.decrement();
             _dispatchedTimeStats.record(dispatched);
 
             if (state.isInitial())
@@ -191,20 +192,21 @@ public void handle(String path, Request baseRequest, HttpServletRequest request,
                 }
                 else
                 {
-                    long d = _requestStats.decrement();
+                    numRequests = _requestStats.decrement();
                     _requestTimeStats.record(dispatched);
                     updateResponse(baseRequest);
-
-                    // If we have no more dispatches, should we signal shutdown?
-                    FutureCallback shutdown = _shutdown.get();
-                    if (shutdown != null)
-                    {
-                        response.flushBuffer();
-                        if (d == 0)
-                            shutdown.succeeded();
-                    }
                 }
             }
+
+            FutureCallback shutdown = _shutdown.get();
+            if (shutdown != null)
+            {
+                response.flushBuffer();
+
+                // If we either have no more requests or dispatches, we can complete shutdown.
+                if (waitForSuspendedRequestsOnShutdown ? (numRequests == 0) : (numDispatches == 0))
+                    shutdown.succeeded();
+            }
         }
     }
 
@@ -257,6 +259,16 @@ protected void doStop() throws Exception
         super.doStop();
     }
 
+    /**
+     * Set whether the graceful shutdown should wait for all requests to complete (including suspended requests)
+     * or whether it should only wait for all the actively dispatched requests to complete.
+     * @param waitForSuspendedRequests true to wait for suspended requests on graceful shutdown.
+     */
+    public void waitForSuspendedRequestsOnShutdown(boolean waitForSuspendedRequests)
+    {
+        this.waitForSuspendedRequestsOnShutdown = waitForSuspendedRequests;
+    }
+
     /**
      * @return the number of requests handled by this handler
      * since {@link #statsReset()} was last called, excluding
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
index 152d1b6c4efd..ed7aed02be76 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
@@ -19,10 +19,12 @@
 package org.eclipse.jetty.server.handler;
 
 import java.io.IOException;
+import java.util.Objects;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.servlet.AsyncContext;
 import javax.servlet.AsyncEvent;
@@ -45,6 +47,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class StatisticsHandlerTest
@@ -422,6 +425,114 @@ public void handle(String path, Request request, HttpServletRequest httpRequest,
         barrier[3].await();
     }
 
+    @Test
+    public void waitForSuspendedRequestTest() throws Exception
+    {
+        CyclicBarrier barrier = new CyclicBarrier(3);
+        final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
+        final CountDownLatch dispatched = new CountDownLatch(1);
+        _statsHandler.waitForSuspendedRequestsOnShutdown(true);
+        _statsHandler.setHandler(new AbstractHandler()
+        {
+            @Override
+            public void handle(String path, Request request, HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws ServletException
+            {
+                request.setHandled(true);
+
+                try
+                {
+                    if (path.contains("async"))
+                    {
+                        asyncHolder.set(request.startAsync());
+                        barrier.await();
+                    }
+                    else
+                    {
+                        barrier.await();
+                        dispatched.await();
+                    }
+                }
+                catch (Exception e)
+                {
+                    throw new ServletException(e);
+                }
+            }
+        });
+        _server.start();
+
+        // One request to block while dispatched other will go async.
+        _connector.executeRequest("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n");
+        _connector.executeRequest("GET /async HTTP/1.1\r\nHost: localhost\r\n\r\n");
+
+        // Ensure the requests have been dispatched and async started.
+        barrier.await();
+        AsyncContext asyncContext = Objects.requireNonNull(asyncHolder.get());
+
+        // Shutdown should timeout as there are two active requests.
+        Future<Void> shutdown = _statsHandler.shutdown();
+        assertThrows(TimeoutException.class, () -> shutdown.get(1, TimeUnit.SECONDS));
+
+        // When the dispatched thread exits we should still be waiting on the async request.
+        dispatched.countDown();
+        assertThrows(TimeoutException.class, () -> shutdown.get(1, TimeUnit.SECONDS));
+
+        // Shutdown should complete only now the AsyncContext is completed.
+        asyncContext.complete();
+        shutdown.get(5, TimeUnit.MILLISECONDS);
+    }
+
+    @Test
+    public void doNotWaitForSuspendedRequestTest() throws Exception
+    {
+        CyclicBarrier barrier = new CyclicBarrier(3);
+        final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
+        final CountDownLatch dispatched = new CountDownLatch(1);
+        _statsHandler.waitForSuspendedRequestsOnShutdown(false);
+        _statsHandler.setHandler(new AbstractHandler()
+        {
+            @Override
+            public void handle(String path, Request request, HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws ServletException
+            {
+                request.setHandled(true);
+
+                try
+                {
+                    if (path.contains("async"))
+                    {
+                        asyncHolder.set(request.startAsync());
+                        barrier.await();
+                    }
+                    else
+                    {
+                        barrier.await();
+                        dispatched.await();
+                    }
+                }
+                catch (Exception e)
+                {
+                    throw new ServletException(e);
+                }
+            }
+        });
+        _server.start();
+
+        // One request to block while dispatched other will go async.
+        _connector.executeRequest("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n");
+        _connector.executeRequest("GET /async HTTP/1.1\r\nHost: localhost\r\n\r\n");
+
+        // Ensure the requests have been dispatched and async started.
+        barrier.await();
+        assertNotNull(asyncHolder.get());
+
+        // Shutdown should timeout as there is a request dispatched.
+        Future<Void> shutdown = _statsHandler.shutdown();
+        assertThrows(TimeoutException.class, () -> shutdown.get(1, TimeUnit.SECONDS));
+
+        // When the dispatched thread exits we should shutdown even though we have a waiting async request.
+        dispatched.countDown();
+        shutdown.get(5, TimeUnit.MILLISECONDS);
+    }
+
     @Test
     public void testSuspendExpire() throws Exception
     {

From 0fae221afab80dda2373bb6bf21ef58b142b957e Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Wed, 19 Aug 2020 21:27:04 +1000
Subject: [PATCH 4/8] Issue #5105 - fix GracefulStopTest expectations from 503s
 to 404s

StatisticsHandler no longer gives 503 responses after shutdown.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../java/org/eclipse/jetty/server/GracefulStopTest.java  | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
index 6cd9a8b7548e..10fd5d12cbc8 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
@@ -305,7 +305,7 @@ public void run()
                         ).getBytes());
                         client2.getOutputStream().flush();
                         String response2 = IO.toString(client2.getInputStream());
-                        assertThat(response2, containsString(" 503 "));
+                        assertThat(response2, containsString(" 404 "));
 
                         now = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
                         Thread.sleep(Math.max(1, end - now));
@@ -330,8 +330,9 @@ public void run()
             assertThat(response, containsString(" 200 OK"));
             assertThat(response, containsString("read 10/10"));
 
-            assertThat(stats.getRequests(), is(2));
-            assertThat(stats.getResponses5xx(), is(1));
+            // The StatisticsHandler was shutdown when it received the second request so does not contribute to the stats.
+            assertThat(stats.getRequests(), is(1));
+            assertThat(stats.getResponses4xx(), is(0));
         }
     }
 
@@ -631,7 +632,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
 
         // Check new connections rejected!
         String unavailable = connector.getResponse("GET / HTTP/1.1\r\nHost:localhost\r\n\r\n");
-        assertThat(unavailable, containsString(" 503 Service Unavailable"));
+        assertThat(unavailable, containsString(" 404 Not Found"));
         assertThat(unavailable, Matchers.containsString("Connection: close"));
 
         // Check completed 200 has close

From c0268c590f118720537e1b72c9a4cb1915cdfc39 Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Mon, 24 Aug 2020 16:17:51 +1000
Subject: [PATCH 5/8] change name of waitForSuspendedRequestsOnShutdown to
 asyncGraceful

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../src/main/config/etc/jetty-stats.xml         |  4 ++--
 jetty-server/src/main/config/modules/stats.mod  |  4 ++--
 .../jetty/server/handler/StatisticsHandler.java | 17 +++++++++--------
 .../eclipse/jetty/server/GracefulStopTest.java  |  1 +
 .../server/handler/StatisticsHandlerTest.java   |  4 ++--
 5 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/jetty-server/src/main/config/etc/jetty-stats.xml b/jetty-server/src/main/config/etc/jetty-stats.xml
index 9423fad2abd4..9a514ab7ef52 100644
--- a/jetty-server/src/main/config/etc/jetty-stats.xml
+++ b/jetty-server/src/main/config/etc/jetty-stats.xml
@@ -6,8 +6,8 @@
   <Call name="insertHandler">
     <Arg>
       <New id="StatsHandler" class="org.eclipse.jetty.server.handler.StatisticsHandler">
-        <Set name="waitForSuspendedRequestsOnShutdown">
-          <Property name="jetty.statistics.waitForSuspendedRequestsOnShutdown" default="true"/>
+        <Set name="asyncGraceful">
+          <Property name="jetty.statistics.asyncGraceful" default="true"/>
         </Set>
       </New>
     </Arg>
diff --git a/jetty-server/src/main/config/modules/stats.mod b/jetty-server/src/main/config/modules/stats.mod
index be551a058d01..23b0f16237ad 100644
--- a/jetty-server/src/main/config/modules/stats.mod
+++ b/jetty-server/src/main/config/modules/stats.mod
@@ -18,5 +18,5 @@ jetty.webapp.addServerClasses+=,-org.eclipse.jetty.servlet.StatisticsServlet
 
 [ini-template]
 
-## If the Graceful shutdown should wait for suspended requests as well as dispatched ones.
-# jetty.statistics.waitForSuspendedRequestsOnShutdown=true
+## If the Graceful shutdown should wait for async requests as well as the currently dispatched ones.
+# jetty.statistics.asyncGraceful=true
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
index 059541fa17a7..9a45a0874e44 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
@@ -66,7 +66,7 @@ public class StatisticsHandler extends HandlerWrapper implements Graceful
     private final LongAdder _responses5xx = new LongAdder();
     private final LongAdder _responsesTotalBytes = new LongAdder();
 
-    private boolean waitForSuspendedRequestsOnShutdown = true;
+    private boolean asyncGraceful = true;
 
     private final Graceful.Shutdown _shutdown = new Graceful.Shutdown()
     {
@@ -113,7 +113,7 @@ public void onComplete(AsyncEvent event) throws IOException
             _asyncWaitStats.decrement();
 
             // If we have no more dispatches, should we signal shutdown?
-            if (numRequests == 0 && waitForSuspendedRequestsOnShutdown)
+            if (numRequests == 0 && asyncGraceful)
             {
                 FutureCallback shutdown = _shutdown.get();
                 if (shutdown != null)
@@ -204,7 +204,7 @@ public void handle(String path, Request baseRequest, HttpServletRequest request,
                 response.flushBuffer();
 
                 // If we either have no more requests or dispatches, we can complete shutdown.
-                if (waitForSuspendedRequestsOnShutdown ? (numRequests == 0) : (numDispatches == 0))
+                if (asyncGraceful ? (numRequests == 0) : (numDispatches == 0))
                     shutdown.succeeded();
             }
         }
@@ -260,13 +260,14 @@ protected void doStop() throws Exception
     }
 
     /**
-     * Set whether the graceful shutdown should wait for all requests to complete (including suspended requests)
-     * or whether it should only wait for all the actively dispatched requests to complete.
-     * @param waitForSuspendedRequests true to wait for suspended requests on graceful shutdown.
+     * Set whether the graceful shutdown should wait for all requests to complete including
+     * async requests which are not currently dispatched, or whether it should only wait for all the
+     * actively dispatched requests to complete.
+     * @param asyncGraceful true to wait for async requests on graceful shutdown.
      */
-    public void waitForSuspendedRequestsOnShutdown(boolean waitForSuspendedRequests)
+    public void setAsyncGraceful(boolean asyncGraceful)
     {
-        this.waitForSuspendedRequestsOnShutdown = waitForSuspendedRequests;
+        this.asyncGraceful = asyncGraceful;
     }
 
     /**
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
index 10fd5d12cbc8..1627cff6f9d4 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
@@ -558,6 +558,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
     public void testCommittedResponsesAreClosed() throws Exception
     {
         Server server = new Server();
+        server.setDumpAfterStart(true);
 
         LocalConnector connector = new LocalConnector(server);
         server.addConnector(connector);
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
index ed7aed02be76..c4bbc702cf9a 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
@@ -431,7 +431,7 @@ public void waitForSuspendedRequestTest() throws Exception
         CyclicBarrier barrier = new CyclicBarrier(3);
         final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
         final CountDownLatch dispatched = new CountDownLatch(1);
-        _statsHandler.waitForSuspendedRequestsOnShutdown(true);
+        _statsHandler.setAsyncGraceful(true);
         _statsHandler.setHandler(new AbstractHandler()
         {
             @Override
@@ -487,7 +487,7 @@ public void doNotWaitForSuspendedRequestTest() throws Exception
         CyclicBarrier barrier = new CyclicBarrier(3);
         final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
         final CountDownLatch dispatched = new CountDownLatch(1);
-        _statsHandler.waitForSuspendedRequestsOnShutdown(false);
+        _statsHandler.setAsyncGraceful(false);
         _statsHandler.setHandler(new AbstractHandler()
         {
             @Override

From 2d5da1fa35d81584ce0f947b1e12b46e98762192 Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Tue, 25 Aug 2020 10:34:59 +1000
Subject: [PATCH 6/8] StatisticsHandler should still return 503 responses on
 shutdown

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 jetty-server/src/main/config/etc/jetty-stats.xml             | 4 +---
 .../org/eclipse/jetty/server/handler/StatisticsHandler.java  | 5 +++++
 .../test/java/org/eclipse/jetty/server/GracefulStopTest.java | 5 ++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/jetty-server/src/main/config/etc/jetty-stats.xml b/jetty-server/src/main/config/etc/jetty-stats.xml
index 9a514ab7ef52..f8fd0ba9dff9 100644
--- a/jetty-server/src/main/config/etc/jetty-stats.xml
+++ b/jetty-server/src/main/config/etc/jetty-stats.xml
@@ -6,9 +6,7 @@
   <Call name="insertHandler">
     <Arg>
       <New id="StatsHandler" class="org.eclipse.jetty.server.handler.StatisticsHandler">
-        <Set name="asyncGraceful">
-          <Property name="jetty.statistics.asyncGraceful" default="true"/>
-        </Set>
+        <Set name="asyncGraceful"><Property name="jetty.statistics.asyncGraceful" default="true"/></Set>
       </New>
     </Arg>
   </Call>
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
index 9a45a0874e44..70650b7a04cc 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
@@ -28,6 +28,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.server.AsyncContextEvent;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.HttpChannelState;
@@ -151,7 +152,11 @@ public void handle(String path, Request baseRequest, HttpServletRequest request,
     {
         Handler handler = getHandler();
         if (handler == null || !isStarted() || isShutdown())
+        {
+            if (!baseRequest.getResponse().isCommitted())
+                response.sendError(HttpStatus.SERVICE_UNAVAILABLE_503);
             return;
+        }
 
         _dispatchedStats.increment();
 
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
index 1627cff6f9d4..902c15a72283 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulStopTest.java
@@ -305,7 +305,7 @@ public void run()
                         ).getBytes());
                         client2.getOutputStream().flush();
                         String response2 = IO.toString(client2.getInputStream());
-                        assertThat(response2, containsString(" 404 "));
+                        assertThat(response2, containsString(" 503 "));
 
                         now = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
                         Thread.sleep(Math.max(1, end - now));
@@ -558,7 +558,6 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
     public void testCommittedResponsesAreClosed() throws Exception
     {
         Server server = new Server();
-        server.setDumpAfterStart(true);
 
         LocalConnector connector = new LocalConnector(server);
         server.addConnector(connector);
@@ -633,7 +632,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
 
         // Check new connections rejected!
         String unavailable = connector.getResponse("GET / HTTP/1.1\r\nHost:localhost\r\n\r\n");
-        assertThat(unavailable, containsString(" 404 Not Found"));
+        assertThat(unavailable, containsString(" 503 Service Unavailable"));
         assertThat(unavailable, Matchers.containsString("Connection: close"));
 
         // Check completed 200 has close

From 8edb7682cdedd3c14717eb8fd424db339273aec9 Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Tue, 25 Aug 2020 10:40:09 +1000
Subject: [PATCH 7/8] DebugHandler should use isAsyncStarted() instead of
 isSuspended()

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../java/org/eclipse/jetty/server/handler/DebugHandler.java | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DebugHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DebugHandler.java
index 2f91bb174d6e..fc110b0f6a29 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DebugHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DebugHandler.java
@@ -61,7 +61,6 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
         final Thread thread = Thread.currentThread();
         final String old_name = thread.getName();
 
-        boolean suspend = false;
         boolean retry = false;
         String name = (String)request.getAttribute("org.eclipse.jetty.thread.name");
         if (name == null)
@@ -103,11 +102,10 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
         finally
         {
             thread.setName(old_name);
-            suspend = baseRequest.getHttpChannelState().isSuspended();
-            if (suspend)
+            if (baseRequest.getHttpChannelState().isAsyncStarted())
             {
                 request.setAttribute("org.eclipse.jetty.thread.name", name);
-                print(name, "SUSPEND");
+                print(name, "ASYNC");
             }
             else
                 print(name, "RESPONSE " + base_response.getStatus() + (ex == null ? "" : ("/" + ex)) + " " + base_response.getContentType());

From a9c90d330915933abc49805364f06cb41079f242 Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Wed, 26 Aug 2020 14:45:12 +1000
Subject: [PATCH 8/8] Issue #5105 - change asyncGraceful to
 gracefulShutdownWaitsForRequests

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../src/main/config/etc/jetty-stats.xml       |  2 +-
 .../src/main/config/modules/stats.mod         |  2 +-
 .../server/handler/StatisticsHandler.java     | 35 ++++++++++++-------
 .../server/handler/StatisticsHandlerTest.java |  4 +--
 4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/jetty-server/src/main/config/etc/jetty-stats.xml b/jetty-server/src/main/config/etc/jetty-stats.xml
index f8fd0ba9dff9..8ba66e60d701 100644
--- a/jetty-server/src/main/config/etc/jetty-stats.xml
+++ b/jetty-server/src/main/config/etc/jetty-stats.xml
@@ -6,7 +6,7 @@
   <Call name="insertHandler">
     <Arg>
       <New id="StatsHandler" class="org.eclipse.jetty.server.handler.StatisticsHandler">
-        <Set name="asyncGraceful"><Property name="jetty.statistics.asyncGraceful" default="true"/></Set>
+        <Set name="gracefulShutdownWaitsForRequests"><Property name="jetty.statistics.gracefulShutdownWaitsForRequests" default="true"/></Set>
       </New>
     </Arg>
   </Call>
diff --git a/jetty-server/src/main/config/modules/stats.mod b/jetty-server/src/main/config/modules/stats.mod
index 23b0f16237ad..a6289c4174bb 100644
--- a/jetty-server/src/main/config/modules/stats.mod
+++ b/jetty-server/src/main/config/modules/stats.mod
@@ -19,4 +19,4 @@ jetty.webapp.addServerClasses+=,-org.eclipse.jetty.servlet.StatisticsServlet
 [ini-template]
 
 ## If the Graceful shutdown should wait for async requests as well as the currently dispatched ones.
-# jetty.statistics.asyncGraceful=true
+# jetty.statistics.gracefulShutdownWaitsForRequests=true
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
index 70650b7a04cc..965895512958 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java
@@ -67,7 +67,7 @@ public class StatisticsHandler extends HandlerWrapper implements Graceful
     private final LongAdder _responses5xx = new LongAdder();
     private final LongAdder _responsesTotalBytes = new LongAdder();
 
-    private boolean asyncGraceful = true;
+    private boolean _gracefulShutdownWaitsForRequests = true;
 
     private final Graceful.Shutdown _shutdown = new Graceful.Shutdown()
     {
@@ -81,25 +81,25 @@ protected FutureCallback newShutdownCallback()
     private final AsyncListener _onCompletion = new AsyncListener()
     {
         @Override
-        public void onStartAsync(AsyncEvent event) throws IOException
+        public void onStartAsync(AsyncEvent event)
         {
             event.getAsyncContext().addListener(this);
         }
 
         @Override
-        public void onTimeout(AsyncEvent event) throws IOException
+        public void onTimeout(AsyncEvent event)
         {
             _expires.increment();
         }
 
         @Override
-        public void onError(AsyncEvent event) throws IOException
+        public void onError(AsyncEvent event)
         {
             _errors.increment();
         }
 
         @Override
-        public void onComplete(AsyncEvent event) throws IOException
+        public void onComplete(AsyncEvent event)
         {
             HttpChannelState state = ((AsyncContextEvent)event).getHttpChannelState();
 
@@ -113,8 +113,7 @@ public void onComplete(AsyncEvent event) throws IOException
 
             _asyncWaitStats.decrement();
 
-            // If we have no more dispatches, should we signal shutdown?
-            if (numRequests == 0 && asyncGraceful)
+            if (numRequests == 0 && _gracefulShutdownWaitsForRequests)
             {
                 FutureCallback shutdown = _shutdown.get();
                 if (shutdown != null)
@@ -207,9 +206,7 @@ public void handle(String path, Request baseRequest, HttpServletRequest request,
             if (shutdown != null)
             {
                 response.flushBuffer();
-
-                // If we either have no more requests or dispatches, we can complete shutdown.
-                if (asyncGraceful ? (numRequests == 0) : (numDispatches == 0))
+                if (_gracefulShutdownWaitsForRequests ? (numRequests == 0) : (numDispatches == 0))
                     shutdown.succeeded();
             }
         }
@@ -268,11 +265,23 @@ protected void doStop() throws Exception
      * Set whether the graceful shutdown should wait for all requests to complete including
      * async requests which are not currently dispatched, or whether it should only wait for all the
      * actively dispatched requests to complete.
-     * @param asyncGraceful true to wait for async requests on graceful shutdown.
+     * @param gracefulShutdownWaitsForRequests true to wait for async requests on graceful shutdown.
+     */
+    public void setGracefulShutdownWaitsForRequests(boolean gracefulShutdownWaitsForRequests)
+    {
+        _gracefulShutdownWaitsForRequests = gracefulShutdownWaitsForRequests;
+    }
+
+    /**
+     * @return whether the graceful shutdown will wait for all requests to complete including
+     * async requests which are not currently dispatched, or whether it will only wait for all the
+     * actively dispatched requests to complete.
+     * @see #getAsyncDispatches()
      */
-    public void setAsyncGraceful(boolean asyncGraceful)
+    @ManagedAttribute("if graceful shutdown will wait for all requests")
+    public boolean getGracefulShutdownWaitsForRequests()
     {
-        this.asyncGraceful = asyncGraceful;
+        return _gracefulShutdownWaitsForRequests;
     }
 
     /**
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
index c4bbc702cf9a..7ce2b5b0718c 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/StatisticsHandlerTest.java
@@ -431,7 +431,7 @@ public void waitForSuspendedRequestTest() throws Exception
         CyclicBarrier barrier = new CyclicBarrier(3);
         final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
         final CountDownLatch dispatched = new CountDownLatch(1);
-        _statsHandler.setAsyncGraceful(true);
+        _statsHandler.setGracefulShutdownWaitsForRequests(true);
         _statsHandler.setHandler(new AbstractHandler()
         {
             @Override
@@ -487,7 +487,7 @@ public void doNotWaitForSuspendedRequestTest() throws Exception
         CyclicBarrier barrier = new CyclicBarrier(3);
         final AtomicReference<AsyncContext> asyncHolder = new AtomicReference<>();
         final CountDownLatch dispatched = new CountDownLatch(1);
-        _statsHandler.setAsyncGraceful(false);
+        _statsHandler.setGracefulShutdownWaitsForRequests(false);
         _statsHandler.setHandler(new AbstractHandler()
         {
             @Override