From 29ffd71b83fedabb9a39cabdb8905d565035bfd8 Mon Sep 17 00:00:00 2001
From: markt <markt@13f79535-47bb-0310-9956-ffa450edef68>
Date: Sun, 5 Sep 2010 20:36:16 +0000
Subject: [PATCH] Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=49802
 Allow the connectors to be stopped via JMX.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@992890 13f79535-47bb-0310-9956-ffa450edef68
---
 java/org/apache/catalina/connector/Connector.java  |  8 ++++
 java/org/apache/catalina/core/StandardService.java | 26 +++++-------
 java/org/apache/coyote/ProtocolHandler.java        |  6 +++
 java/org/apache/coyote/ajp/AjpAprProcessor.java    |  6 +--
 java/org/apache/coyote/ajp/AjpAprProtocol.java     | 13 +++++-
 java/org/apache/coyote/ajp/AjpProcessor.java       |  4 +-
 java/org/apache/coyote/ajp/AjpProtocol.java        | 13 +++++-
 java/org/apache/coyote/ajp/LocalStrings.properties |  2 +
 .../coyote/http11/AbstractHttp11Protocol.java      | 14 ++++++-
 .../apache/coyote/http11/Http11AprProcessor.java   | 18 ++++----
 .../apache/coyote/http11/Http11NioProcessor.java   | 16 ++++---
 java/org/apache/coyote/http11/Http11Processor.java |  4 +-
 .../apache/coyote/http11/LocalStrings.properties   |  2 +
 .../apache/tomcat/util/net/AbstractEndpoint.java   | 33 +++++++++++++--
 java/org/apache/tomcat/util/net/AprEndpoint.java   | 32 ++++----------
 java/org/apache/tomcat/util/net/JIoEndpoint.java   | 22 ++++------
 java/org/apache/tomcat/util/net/NioEndpoint.java   | 49 ++--------------------
 webapps/docs/changelog.xml                         |  4 ++
 18 files changed, 138 insertions(+), 134 deletions(-)

diff --git a/java/org/apache/catalina/connector/Connector.java b/java/org/apache/catalina/connector/Connector.java
index 47328d505..f100a539b 100644
--- a/java/org/apache/catalina/connector/Connector.java
+++ b/java/org/apache/catalina/connector/Connector.java
@@ -916,6 +916,14 @@ public class Connector extends LifecycleMBeanBase  {
 
         setState(LifecycleState.STOPPING);
 
+        try {
+            protocolHandler.stop();
+        } catch (Exception e) {
+            throw new LifecycleException
+                (sm.getString
+                 ("coyoteConnector.protocolHandlerStopFailed", e));
+        }
+
         // MapperListener doesn't follow Lifecycle conventions
         mapperListener.destroy();
     }
diff --git a/java/org/apache/catalina/core/StandardService.java b/java/org/apache/catalina/core/StandardService.java
index 13f97400a..e3774b1c8 100644
--- a/java/org/apache/catalina/core/StandardService.java
+++ b/java/org/apache/catalina/core/StandardService.java
@@ -377,13 +377,13 @@ public class StandardService extends LifecycleMBeanBase implements Service {
 
     /**
      * Retrieves executor by name, null if not found
-     * @param name String
+     * @param executorName String
      * @return Executor
      */
-    public Executor getExecutor(String name) {
+    public Executor getExecutor(String executorName) {
         synchronized (executors) {
             for (Executor executor: executors) {
-                if (name.equals(executor.getName()))
+                if (executorName.equals(executor.getName()))
                     return executor;
             }
         }
@@ -462,7 +462,7 @@ public class StandardService extends LifecycleMBeanBase implements Service {
     @Override
     protected void stopInternal() throws LifecycleException {
 
-        // Stop our defined Connectors first
+        // Pause connectors first
         synchronized (connectors) {
             for (Connector connector: connectors) {
                 try {
@@ -475,13 +475,6 @@ public class StandardService extends LifecycleMBeanBase implements Service {
             }
         }
 
-        // Heuristic: Sleep for a while to ensure pause of the connector
-        try {
-            Thread.sleep(1000);
-        } catch (InterruptedException e) {
-            // Ignore
-        }
-
         if(log.isInfoEnabled())
             log.info(sm.getString("standardService.stop.name", this.name));
         setState(LifecycleState.STOPPING);
@@ -492,14 +485,15 @@ public class StandardService extends LifecycleMBeanBase implements Service {
                 container.stop();
             }
         }
-        // FIXME pero -- Why container stop first? KeepAlive connections can send request! 
-        // Stop our defined Connectors first
+
+        // Now stop the connectors
         synchronized (connectors) {
             for (Connector connector: connectors) {
-                if (LifecycleState.INITIALIZED.equals(
+                if (!LifecycleState.STARTED.equals(
                         connector.getState())) {
-                    // If Service fails to start, connectors may not have been
-                    // started
+                    // Connectors only need stopping if they are currently
+                    // started. They may have failed to start or may have been
+                    // stopped (e.g. via a JMX call)
                     continue;
                 }
                 try {
diff --git a/java/org/apache/coyote/ProtocolHandler.java b/java/org/apache/coyote/ProtocolHandler.java
index 25eea185c..8b2f608f0 100644
--- a/java/org/apache/coyote/ProtocolHandler.java
+++ b/java/org/apache/coyote/ProtocolHandler.java
@@ -82,6 +82,12 @@ public interface ProtocolHandler {
 
 
     /**
+     * Stop the protocol.
+     */
+    public void stop() throws Exception;
+
+
+    /**
      * Destroy the protocol (optional).
      */
     public void destroy() throws Exception;
diff --git a/java/org/apache/coyote/ajp/AjpAprProcessor.java b/java/org/apache/coyote/ajp/AjpAprProcessor.java
index 8eec76e36..103b147b8 100644
--- a/java/org/apache/coyote/ajp/AjpAprProcessor.java
+++ b/java/org/apache/coyote/ajp/AjpAprProcessor.java
@@ -378,7 +378,7 @@ public class AjpAprProcessor implements ActionHook {
         boolean openSocket = true;
         boolean keptAlive = false;
 
-        while (started && !error) {
+        while (started && !error && !endpoint.isPaused()) {
 
             // Parsing the request header
             try {
@@ -474,14 +474,14 @@ public class AjpAprProcessor implements ActionHook {
         }
 
         // Add the socket to the poller
-        if (!error) {
+        if (!error && !endpoint.isPaused()) {
             endpoint.getPoller().add(socket);
         } else {
             openSocket = false;
         }
 
         rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
-        if (!async || error)
+        if (!async || error || endpoint.isPaused())
              recycle();
 
         return openSocket;
diff --git a/java/org/apache/coyote/ajp/AjpAprProtocol.java b/java/org/apache/coyote/ajp/AjpAprProtocol.java
index 7c639368f..e8184a667 100644
--- a/java/org/apache/coyote/ajp/AjpAprProtocol.java
+++ b/java/org/apache/coyote/ajp/AjpAprProtocol.java
@@ -218,9 +218,20 @@ public class AjpAprProtocol
             log.info(sm.getString("ajpprotocol.resume", getName()));
     }
 
-    public void destroy() throws Exception {
+    public void stop() throws Exception {
+        try {
+            endpoint.stop();
+        } catch (Exception ex) {
+            log.error(sm.getString("ajpprotocol.endpoint.stoperror"), ex);
+            throw ex;
+        }
         if (log.isInfoEnabled())
             log.info(sm.getString("ajpprotocol.stop", getName()));
+    }
+
+    public void destroy() throws Exception {
+        if (log.isInfoEnabled())
+            log.info(sm.getString("ajpprotocol.destroy", getName()));
         endpoint.destroy();
         if (tpOname!=null)
             Registry.getRegistry(null, null).unregisterComponent(tpOname);
diff --git a/java/org/apache/coyote/ajp/AjpProcessor.java b/java/org/apache/coyote/ajp/AjpProcessor.java
index 906c5d483..9b983ebbe 100644
--- a/java/org/apache/coyote/ajp/AjpProcessor.java
+++ b/java/org/apache/coyote/ajp/AjpProcessor.java
@@ -383,7 +383,7 @@ public class AjpProcessor implements ActionHook {
         // Error flag
         error = false;
 
-        while (started && !error) {
+        while (started && !error && !endpoint.isPaused()) {
 
             // Parsing the request header
             try {
@@ -483,7 +483,7 @@ public class AjpProcessor implements ActionHook {
             recycle();
 
         }
-        if (async && !error) {
+        if (async && !error && !endpoint.isPaused()) {
             rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
             return SocketState.LONG;
         } else {
diff --git a/java/org/apache/coyote/ajp/AjpProtocol.java b/java/org/apache/coyote/ajp/AjpProtocol.java
index 2304de9eb..584a8b383 100644
--- a/java/org/apache/coyote/ajp/AjpProtocol.java
+++ b/java/org/apache/coyote/ajp/AjpProtocol.java
@@ -220,9 +220,20 @@ public class AjpProtocol
             log.info(sm.getString("ajpprotocol.resume", getName()));
     }
 
-    public void destroy() throws Exception {
+    public void stop() throws Exception {
+        try {
+            endpoint.stop();
+        } catch (Exception ex) {
+            log.error(sm.getString("ajpprotocol.endpoint.stoperror"), ex);
+            throw ex;
+        }
         if (log.isInfoEnabled())
             log.info(sm.getString("ajpprotocol.stop", getName()));
+    }
+
+    public void destroy() throws Exception {
+        if (log.isInfoEnabled())
+            log.info(sm.getString("ajpprotocol.destroy", getName()));
         endpoint.destroy();
         if (tpOname!=null)
             Registry.getRegistry(null, null).unregisterComponent(tpOname);
diff --git a/java/org/apache/coyote/ajp/LocalStrings.properties b/java/org/apache/coyote/ajp/LocalStrings.properties
index 1ce513e35..4965bfc26 100644
--- a/java/org/apache/coyote/ajp/LocalStrings.properties
+++ b/java/org/apache/coyote/ajp/LocalStrings.properties
@@ -23,8 +23,10 @@
 # AjpAprProtocol
 #
 
+ajpprotocol.destroy=Destroying Coyote AJP/1.3 on {0}
 ajpprotocol.endpoint.initerror=Error initializing endpoint
 ajpprotocol.endpoint.starterror=Error starting endpoint
+ajpprotocol.endpoint.stoperror=Error stopping endpoint
 ajpprotocol.init=Initializing Coyote AJP/1.3 on {0}
 ajpprotocol.proto.error=Error reading request, ignored
 ajpprotocol.getattribute=Attribute {0}
diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
index 7182e3071..1cd32a99f 100644
--- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
+++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
@@ -148,9 +148,21 @@ public abstract class AbstractHttp11Protocol implements ProtocolHandler, MBeanRe
     }
 
     @Override
-    public void destroy() throws Exception {
+    public void stop() throws Exception {
+        try {
+            endpoint.stop();
+        } catch (Exception ex) {
+            getLog().error(sm.getString("http11protocol.endpoint.stoperror"), ex);
+            throw ex;
+        }
         if(getLog().isInfoEnabled())
             getLog().info(sm.getString("http11protocol.stop", getName()));
+    }
+
+    @Override
+    public void destroy() throws Exception {
+        if(getLog().isInfoEnabled())
+            getLog().info(sm.getString("http11protocol.destroy", getName()));
         endpoint.destroy();
         if( tpOname!=null )
             Registry.getRegistry(null, null).unregisterComponent(tpOname);
diff --git a/java/org/apache/coyote/http11/Http11AprProcessor.java b/java/org/apache/coyote/http11/Http11AprProcessor.java
index d7840a74c..ac5a0f19b 100644
--- a/java/org/apache/coyote/http11/Http11AprProcessor.java
+++ b/java/org/apache/coyote/http11/Http11AprProcessor.java
@@ -220,7 +220,7 @@ public class Http11AprProcessor extends AbstractHttp11Processor implements Actio
         boolean keptAlive = false;
         boolean openSocket = false;
 
-        while (!error && keepAlive && !comet && !async) {
+        while (!error && keepAlive && !comet && !async && !endpoint.isPaused()) {
 
             // Parsing the request header
             try {
@@ -338,15 +338,13 @@ public class Http11AprProcessor extends AbstractHttp11Processor implements Actio
 
         rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
 
-        if (comet  || async) {
-            if (error) {
-                inputBuffer.nextRequest();
-                outputBuffer.nextRequest();
-                recycle();
-                return SocketState.CLOSED;
-            } else {
-                return SocketState.LONG;
-            }
+        if (error || endpoint.isPaused()) {
+            inputBuffer.nextRequest();
+            outputBuffer.nextRequest();
+            recycle();
+            return SocketState.CLOSED;
+        } else if (comet  || async) {
+            return SocketState.LONG;
         } else {
             recycle();
             return (openSocket) ? SocketState.OPEN : SocketState.CLOSED;
diff --git a/java/org/apache/coyote/http11/Http11NioProcessor.java b/java/org/apache/coyote/http11/Http11NioProcessor.java
index d86758bd9..7b5a66289 100644
--- a/java/org/apache/coyote/http11/Http11NioProcessor.java
+++ b/java/org/apache/coyote/http11/Http11NioProcessor.java
@@ -299,7 +299,7 @@ public class Http11NioProcessor extends AbstractHttp11Processor implements Actio
         boolean recycle = true;
         final KeyAttachment ka = (KeyAttachment)socket.getAttachment(false);
         
-        while (!error && keepAlive && !comet && !async) {
+        while (!error && keepAlive && !comet && !async && !endpoint.isPaused()) {
             //always default to our soTimeout
             ka.setTimeout(soTimeout);
             // Parsing the request header
@@ -448,15 +448,13 @@ public class Http11NioProcessor extends AbstractHttp11Processor implements Actio
         }//while
 
         rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
-        if (comet || async) {
-            if (error) {
-                recycle();
-                return SocketState.CLOSED;
-            } else {
-                return SocketState.LONG;
-            }
+        if (error || endpoint.isPaused()) {
+            recycle();
+            return SocketState.CLOSED;
+        } else if (comet || async) {
+            return SocketState.LONG;
         } else {
-            if ( recycle ) {
+            if (recycle) {
                 recycle();
             }
             //return (openSocket) ? (SocketState.OPEN) : SocketState.CLOSED;
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
index 586717735..5b6a41fac 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -181,7 +181,7 @@ public class Http11Processor extends AbstractHttp11Processor implements ActionHo
 
         boolean keptAlive = socketWrapper.isKeptAlive();
 
-        while (started && !error && keepAlive) {
+        while (started && !error && keepAlive && !endpoint.isPaused()) {
 
             // Parsing the request header
             try {
@@ -309,7 +309,7 @@ public class Http11Processor extends AbstractHttp11Processor implements ActionHo
         }
 
         rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
-        if (error) {
+        if (error || endpoint.isPaused()) {
             recycle();
             return SocketState.CLOSED;
         } else if (async) {
diff --git a/java/org/apache/coyote/http11/LocalStrings.properties b/java/org/apache/coyote/http11/LocalStrings.properties
index 8c47a61fb..ad1081a1d 100644
--- a/java/org/apache/coyote/http11/LocalStrings.properties
+++ b/java/org/apache/coyote/http11/LocalStrings.properties
@@ -23,8 +23,10 @@
 # Http11Protocol
 #
 
+http11protocol.destroy=Destroying Coyote HTTP/1.1 on {0}
 http11protocol.endpoint.initerror=Error initializing endpoint
 http11protocol.endpoint.starterror=Error starting endpoint
+http11protocol.endpoint.stoperror=Error stopping endpoint
 http11protocol.init=Initializing Coyote HTTP/1.1 on {0}
 http11protocol.proto.error=Error reading request, ignored
 http11protocol.proto.ioexception.debug=IOException reading request
diff --git a/java/org/apache/tomcat/util/net/AbstractEndpoint.java b/java/org/apache/tomcat/util/net/AbstractEndpoint.java
index 63b31066f..43116e46c 100644
--- a/java/org/apache/tomcat/util/net/AbstractEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AbstractEndpoint.java
@@ -430,11 +430,38 @@ public abstract class AbstractEndpoint {
         }
     }    
     
-    public abstract void pause();
-    public abstract void resume();
+    
+    public abstract void init() throws Exception;
     public abstract void start() throws Exception;
+    
+    /**
+     * Pause the endpoint, which will stop it accepting new connections.
+     */
+    public void pause() {
+        if (running && !paused) {
+            paused = true;
+            unlockAccept();
+            // Heuristic: Sleep for a while to ensure pause of the endpoint
+            try {
+                Thread.sleep(1000);
+            } catch (InterruptedException e) {
+                // Ignore
+            }
+        }
+    }
+    
+    /**
+     * Resume the endpoint, which will make it start accepting new connections
+     * again.
+     */
+    public void resume() {
+        if (running) {
+            paused = false;
+        }
+    }
+    
+    public abstract void stop() throws Exception;
     public abstract void destroy() throws Exception;
-    public abstract void init() throws Exception;
     
     public String adjustRelativePath(String path, String relativeTo) {
         File f = new File(path);
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index 0ffa4daef..29232ba6c 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -640,33 +640,12 @@ public class AprEndpoint extends AbstractEndpoint {
     }
 
     /**
-     * Pause the endpoint, which will make it stop accepting new sockets.
-     */
-    @Override
-    public void pause() {
-        if (running && !paused) {
-            paused = true;
-            unlockAccept();
-        }
-    }
-
-
-    /**
-     * Resume the endpoint, which will make it start accepting new sockets
-     * again.
-     */
-    @Override
-    public void resume() {
-        if (running) {
-            paused = false;
-        }
-    }
-
-
-    /**
      * Stop the endpoint. This will cause all processing threads to stop.
      */
     public void stop() {
+        if (!paused) {
+            pause();
+        }
         if (running) {
             running = false;
             unlockAccept();
@@ -822,7 +801,10 @@ public class AprEndpoint extends AbstractEndpoint {
      */
     protected boolean processSocketWithOptions(long socket) {
         try {
-            getExecutor().execute(new SocketWithOptionsProcessor(socket));
+            // During shutdown, executor may be null - avoid NPE
+            if (running) {
+                getExecutor().execute(new SocketWithOptionsProcessor(socket));
+            }
         } catch (RejectedExecutionException x) {
             log.warn("Socket processing request was rejected for:"+socket,x);
             return false;
diff --git a/java/org/apache/tomcat/util/net/JIoEndpoint.java b/java/org/apache/tomcat/util/net/JIoEndpoint.java
index d14f2c428..0853ffb6b 100644
--- a/java/org/apache/tomcat/util/net/JIoEndpoint.java
+++ b/java/org/apache/tomcat/util/net/JIoEndpoint.java
@@ -409,21 +409,10 @@ public class JIoEndpoint extends AbstractEndpoint {
     }
 
     @Override
-    public void pause() {
-        if (running && !paused) {
-            paused = true;
-            unlockAccept();
-        }
-    }
-
-    @Override
-    public void resume() {
-        if (running) {
-            paused = false;
-        }
-    }
-
     public void stop() {
+        if (!paused) {
+            pause();
+        }
         if (running) {
             running = false;
             unlockAccept();
@@ -494,7 +483,10 @@ public class JIoEndpoint extends AbstractEndpoint {
         try {
             SocketWrapper<Socket> wrapper = new SocketWrapper<Socket>(socket);
             wrapper.setKeepAliveLeft(getMaxKeepAliveRequests());
-            getExecutor().execute(new SocketProcessor(wrapper));
+            // During shutdown, executor may be null - avoid NPE
+            if (running) {
+                getExecutor().execute(new SocketProcessor(wrapper));
+            }
         } catch (RejectedExecutionException x) {
             log.warn("Socket processing request was rejected for:"+socket,x);
             return false;
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
index adeb61d6f..d5cadd7a1 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -437,28 +437,6 @@ public class NioEndpoint extends AbstractEndpoint {
     }
 
 
-    /**
-     * Return the state of the endpoint.
-     *
-     * @return true if the endpoint is running, false otherwise
-     */
-    @Override
-    public boolean isRunning() {
-        return running;
-    }
-
-
-    /**
-     * Return the state of the endpoint.
-     *
-     * @return true if the endpoint is paused, false otherwise
-     */
-    @Override
-    public boolean isPaused() {
-        return paused;
-    }
-
-
     // ----------------------------------------------- Public Lifecycle Methods
 
 
@@ -595,33 +573,12 @@ public class NioEndpoint extends AbstractEndpoint {
 
 
     /**
-     * Pause the endpoint, which will make it stop accepting new sockets.
-     */
-    @Override
-    public void pause() {
-        if (running && !paused) {
-            paused = true;
-            unlockAccept();
-        }
-    }
-
-
-    /**
-     * Resume the endpoint, which will make it start accepting new sockets
-     * again.
-     */
-    @Override
-    public void resume() {
-        if (running) {
-            paused = false;
-        }
-    }
-
-
-    /**
      * Stop the endpoint. This will cause all processing threads to stop.
      */
     public void stop() {
+        if (!paused) {
+            pause();
+        }
         if (running) {
             running = false;
             unlockAccept();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1c075d4eb..b906b88c1 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -143,6 +143,10 @@
         Make sure async timeout thread is stopped when the connector is stopped.
         (markt)
       </fix>
+      <fix>
+        <bug>49802</bug>: Re-factor connector pause, stop and destroy methods so
+        that calling any of those methods has the expected results. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">
-- 
2.11.0