[tomcat] branch 9.0.x updated: Fix BZ 64080 - graceful close

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch 9.0.x updated: Fix BZ 64080 - graceful close

markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/9.0.x by this push:
     new b1133ad  Fix BZ 64080 - graceful close
b1133ad is described below

commit b1133ada73a48b738793de1827ae4ca9735babb3
Author: Mark Thomas <[hidden email]>
AuthorDate: Thu Nov 19 11:06:55 2020 +0000

    Fix BZ 64080 - graceful close
   
    https://bz.apache.org/bugzilla/show_bug.cgi?id=64080
---
 java/org/apache/catalina/core/StandardService.java | 35 ++++++++++++--
 java/org/apache/coyote/AbstractProtocol.java       |  8 ++++
 java/org/apache/coyote/LocalStrings.properties     |  1 +
 java/org/apache/coyote/ProtocolHandler.java        | 13 +++++
 .../apache/tomcat/util/net/AbstractEndpoint.java   | 56 +++++++++++++++++++++-
 webapps/docs/changelog.xml                         |  7 +++
 webapps/docs/config/service.xml                    | 10 ++++
 7 files changed, 123 insertions(+), 7 deletions(-)

diff --git a/java/org/apache/catalina/core/StandardService.java b/java/org/apache/catalina/core/StandardService.java
index 05965ca..5597cb0 100644
--- a/java/org/apache/catalina/core/StandardService.java
+++ b/java/org/apache/catalina/core/StandardService.java
@@ -105,8 +105,21 @@ public class StandardService extends LifecycleMBeanBase implements Service {
     protected final MapperListener mapperListener = new MapperListener(this);
 
 
+    private long gracefulStopAwaitMillis = 0;
+
+
     // ------------------------------------------------------------- Properties
 
+    public long getGracefulStopAwaitMillis() {
+        return gracefulStopAwaitMillis;
+    }
+
+
+    public void setGracefulStopAwaitMillis(long gracefulStopAwaitMillis) {
+        this.gracefulStopAwaitMillis = gracefulStopAwaitMillis;
+    }
+
+
     @Override
     public Mapper getMapper() {
         return mapper;
@@ -453,21 +466,33 @@ public class StandardService extends LifecycleMBeanBase implements Service {
     @Override
     protected void stopInternal() throws LifecycleException {
 
-        // Pause connectors first
         synchronized (connectorsLock) {
+            // Initiate a graceful stop for each connector
+            // This will only work if the bindOnInit==false which is not the
+            // default.
             for (Connector connector: connectors) {
-                connector.pause();
-                // Close server socket if bound on start
-                // Note: test is in AbstractEndpoint
                 connector.getProtocolHandler().closeServerSocketGraceful();
             }
+
+            // Wait for the graceful shutdown to complete
+            long waitMillis = gracefulStopAwaitMillis;
+            if (waitMillis > 0) {
+                for (Connector connector: connectors) {
+                    waitMillis = connector.getProtocolHandler().awaitConnectionsClose(waitMillis);
+                }
+            }
+
+            // Pause the connectors
+            for (Connector connector: connectors) {
+                connector.pause();
+            }
         }
 
         if(log.isInfoEnabled())
             log.info(sm.getString("standardService.stop.name", this.name));
         setState(LifecycleState.STOPPING);
 
-        // Stop our defined Container second
+        // Stop our defined Container once the Connectors are all paused
         if (engine != null) {
             synchronized (engine) {
                 engine.stop();
diff --git a/java/org/apache/coyote/AbstractProtocol.java b/java/org/apache/coyote/AbstractProtocol.java
index c226b72..089a07c 100644
--- a/java/org/apache/coyote/AbstractProtocol.java
+++ b/java/org/apache/coyote/AbstractProtocol.java
@@ -734,6 +734,14 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
     }
 
 
+    @Override
+    public long awaitConnectionsClose(long waitMillis) {
+        getLog().info(sm.getString("abstractProtocol.closeConnectionsAwait",
+                Long.valueOf(waitMillis), getName()));
+        return endpoint.awaitConnectionsClose(waitMillis);
+    }
+
+
     private void logPortOffset() {
         if (getPort() != getPortWithOffset()) {
             getLog().info(sm.getString("abstractProtocolHandler.portOffset", getName(),
diff --git a/java/org/apache/coyote/LocalStrings.properties b/java/org/apache/coyote/LocalStrings.properties
index 3009a36..595cfb2 100644
--- a/java/org/apache/coyote/LocalStrings.properties
+++ b/java/org/apache/coyote/LocalStrings.properties
@@ -34,6 +34,7 @@ abstractProcessor.pushrequest.notsupported=Server push requests are not supporte
 abstractProcessor.setErrorState=Error state [{0}] reported while processing request
 abstractProcessor.socket.ssl=Exception getting SSL attributes
 
+abstractProtocol.closeConnectionsAwait=Waiting [{0}] milliseconds for existing connections to [{1}] to complete and close.
 abstractProtocol.mbeanDeregistrationFailed=Failed to deregister MBean named [{0}] from MBean server [{1}]
 abstractProtocol.processorRegisterError=Error registering request processor
 abstractProtocol.processorUnregisterError=Error unregistering request processor
diff --git a/java/org/apache/coyote/ProtocolHandler.java b/java/org/apache/coyote/ProtocolHandler.java
index 34bd47f..0b884d3 100644
--- a/java/org/apache/coyote/ProtocolHandler.java
+++ b/java/org/apache/coyote/ProtocolHandler.java
@@ -136,6 +136,19 @@ public interface ProtocolHandler {
 
 
     /**
+     * Wait for the client connections to the server to close gracefully. The
+     * method will return when all of the client connections have closed or the
+     * method has been waiting for {@code waitTimeMillis}.
+     *
+     * @param waitMillis    The maximum time to wait in milliseconds for the
+     *                      client connections to close.
+     *
+     * @return The wait time, if any remaining when the method returned
+     */
+    public long awaitConnectionsClose(long waitMillis);
+
+
+    /**
      * Requires APR/native library
      *
      * @return <code>true</code> if this Protocol Handler requires the
diff --git a/java/org/apache/tomcat/util/net/AbstractEndpoint.java b/java/org/apache/tomcat/util/net/AbstractEndpoint.java
index fdb172d..ef4618b 100644
--- a/java/org/apache/tomcat/util/net/AbstractEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AbstractEndpoint.java
@@ -135,7 +135,20 @@ public abstract class AbstractEndpoint<S,U> {
     }
 
     protected enum BindState {
-        UNBOUND, BOUND_ON_INIT, BOUND_ON_START, SOCKET_CLOSED_ON_STOP
+        UNBOUND(false),
+        BOUND_ON_INIT(true),
+        BOUND_ON_START(true),
+        SOCKET_CLOSED_ON_STOP(false);
+
+        private final boolean bound;
+
+        private BindState(boolean bound) {
+            this.bound = bound;
+        }
+
+        public boolean isBound() {
+            return bound;
+        }
     }
 
 
@@ -740,7 +753,12 @@ public abstract class AbstractEndpoint<S,U> {
      */
     private int maxKeepAliveRequests=100; // as in Apache HTTPD server
     public int getMaxKeepAliveRequests() {
-        return maxKeepAliveRequests;
+        // Disable keep-alive if the server socket is not bound
+        if (bindState.isBound()) {
+            return maxKeepAliveRequests;
+        } else {
+            return 1;
+        }
     }
     public void setMaxKeepAliveRequests(int maxKeepAliveRequests) {
         this.maxKeepAliveRequests = maxKeepAliveRequests;
@@ -1331,6 +1349,16 @@ public abstract class AbstractEndpoint<S,U> {
      */
     public final void closeServerSocketGraceful() {
         if (bindState == BindState.BOUND_ON_START) {
+            // Stop accepting new connections
+            acceptor.stop(-1);
+            // Release locks that may be preventing the acceptor from stopping
+            releaseConnectionLatch();
+            unlockAccept();
+            // Signal to any multiplexed protocols (HTTP/2) that they may wish
+            // to stop accepting new streams
+            getHandler().pause();
+            // Update the bindState. This has the side-effect of disabling
+            // keep-alive for any in-progress connections
             bindState = BindState.SOCKET_CLOSED_ON_STOP;
             try {
                 doCloseServerSocket();
@@ -1342,6 +1370,30 @@ public abstract class AbstractEndpoint<S,U> {
 
 
     /**
+     * Wait for the client connections to the server to close gracefully. The
+     * method will return when all of the client connections have closed or the
+     * method has been waiting for {@code waitTimeMillis}.
+     *
+     * @param waitMillis    The maximum time to wait in milliseconds for the
+     *                      client connections to close.
+     *
+     * @return The wait time, if any remaining when the method returned
+     */
+    public final long awaitConnectionsClose(long waitMillis) {
+        while (waitMillis > 0 && !connections.isEmpty()) {
+            try {
+                Thread.sleep(50);
+                waitMillis -= 50;
+            } catch (InterruptedException e) {
+                Thread.interrupted();
+                waitMillis = 0;
+            }
+        }
+        return waitMillis;
+    }
+
+
+    /**
      * Actually close the server socket but don't perform any other clean-up.
      *
      * @throws IOException If an error occurs closing the socket
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 398046c..cb963b6 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -106,6 +106,13 @@
 <section name="Tomcat 9.0.41 (markt)" rtext="in development">
   <subsection name="Catalina">
     <changelog>
+      <add>
+        <bug>64080</bug>: Enhance the graceful shutdown feature. Includes a new
+        option for <code>StandardService</code>,
+        <code>gracefulStopAwaitMillis</code>, that allows a time to be
+        specified to wait for client connections to complete and close before
+        the Container hierarchy is stopped. (markt)
+      </add>
       <fix>
         <bug>64921</bug>: Ensure that the <code>LoadBalancerDrainingValve</code>
         uses the correct setting for the secure attribute for any session
diff --git a/webapps/docs/config/service.xml b/webapps/docs/config/service.xml
index bb8d94d..5fd0acd 100644
--- a/webapps/docs/config/service.xml
+++ b/webapps/docs/config/service.xml
@@ -80,6 +80,16 @@
   common attributes listed above):</p>
 
   <attributes>
+
+    <attribute name="gracefulStopAwaitMillis" required="false">
+      <p>The time to wait, in milliseconds, when stopping the Service for the
+      client connections to finish processing and close before the Service's
+      container hierarchy is stopped. The wait only applies to Connectors
+      configured with a <code>bindOnInit</code> value of <code>false</code>.
+      Any value of zero or less means there will be no wait. If not specified,
+      the default value of zero will be used.</p>
+    </attribute>
+
   </attributes>
 
   </subsection>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]