OF-883: Prevent multiple closes of the same connection.

When a connection is closed, several events can be involved (and can be triggered).
Some of these events will attempt to close the connection (if it hadn't been already).
This, at best, least to multiple invocations of close-listeners. At worst, a loop of
close-calls is created.

This commit replaces the two-way boolean that guards closure (isClosed) with a
three-way guard. Now, a distinction is made between between a connection that is
closed, and one that is closing.

Additionally, some null pointer guards have been added, as I've seen those pop up in
my local logs during development.

src/java/org/jivesoftware/openfire/nio/ConnectionHandler.java

@@ -109,18 +109,18 @@ public abstract class ConnectionHandler extends IoHandlerAdapter {
 
     @Override
     public void inputClosed( IoSession session ) throws Exception {
-        // Get the connection for this session
-        Connection connection = (Connection) session.getAttribute(CONNECTION);
-        // Inform the connection that it was closed
-        connection.close();
+        final Connection connection = (Connection) session.getAttribute(CONNECTION);
+        if ( connection != null ) {
+            connection.close();
+        }
     }
 
     @Override
-	public void sessionClosed(IoSession session) throws Exception {
-        // Get the connection for this session
-        Connection connection = (Connection) session.getAttribute(CONNECTION);
-        // Inform the connection that it was closed
-        connection.close();
+    public void sessionClosed(IoSession session) throws Exception {
+        final Connection connection = (Connection) session.getAttribute(CONNECTION);
+        if ( connection != null ) {
+            connection.close();
+        }
     }
 
     /**
@@ -128,31 +128,32 @@ public abstract class ConnectionHandler extends IoHandlerAdapter {
 	 * session idle time as specified by {@link #getMaxIdleTime()}. This method
 	 * will be invoked each time that such a period passes (even if no IO has
 	 * occurred in between).
-	 * 
+	 *
 	 * Openfire will disconnect a session the second time this method is
 	 * invoked, if no IO has occurred between the first and second invocation.
 	 * This allows extensions of this class to use the first invocation to check
 	 * for livelyness of the MINA session (e.g by polling the remote entity, as
 	 * {@link ClientConnectionHandler} does).
-	 * 
-	 * @see org.apache.mina.common.IoHandlerAdapter#sessionIdle(org.apache.mina.common.IoSession,
-	 *      org.apache.mina.common.IdleStatus)
+	 *
+	 * @see IoHandlerAdapter#sessionIdle(IoSession, IdleStatus)
 	 */
     @Override
 	public void sessionIdle(IoSession session, IdleStatus status) throws Exception {
         if (session.getIdleCount(status) > 1) {
             // Get the connection for this session
             final Connection connection = (Connection) session.getAttribute(CONNECTION);
-	        // Close idle connection
-	        if (Log.isDebugEnabled()) {
-	            Log.debug("ConnectionHandler: Closing connection that has been idle: " + connection);
-	        }
-	        connection.close();
+            if (connection != null) {
+                // Close idle connection
+                if (Log.isDebugEnabled()) {
+                    Log.debug("ConnectionHandler: Closing connection that has been idle: " + connection);
+                }
+                connection.close();
+            }
         }
     }
 
     @Override
-	public void exceptionCaught(IoSession session, Throwable cause) throws Exception {
+    public void exceptionCaught(IoSession session, Throwable cause) throws Exception {
         Log.warn("Closing connection due to exception in session: " + session, cause);
 
         try {
@@ -191,13 +192,16 @@ public abstract class ConnectionHandler extends IoHandlerAdapter {
             handler.process((String) message, parser);
         } catch (Exception e) {
             Log.error("Closing connection due to error while processing message: " + message, e);
-            Connection connection = (Connection) session.getAttribute(CONNECTION);
-            connection.close();
+            final Connection connection = (Connection) session.getAttribute(CONNECTION);
+            if ( connection != null ) {
+                connection.close();
+            }
+
         }
     }
 
     @Override
-	public void messageSent(IoSession session, Object message) throws Exception {
+    public void messageSent(IoSession session, Object message) throws Exception {
         super.messageSent(session, message);
         // Update counter of written btyes
         updateWrittenBytesCounter(session);

src/java/org/jivesoftware/openfire/nio/NIOConnection.java

@@ -77,6 +77,8 @@ public class NIOConnection implements Connection {
 
 	private static final Logger Log = LoggerFactory.getLogger(NIOConnection.class);
 
+    public enum State { RUNNING, CLOSING, CLOSED }
+
     /**
      * The utf-8 charset for decoding and encoding XMPP packet streams.
      */
@@ -109,13 +111,14 @@ public class NIOConnection implements Connection {
      */
     private CompressionPolicy compressionPolicy = CompressionPolicy.disabled;
     private static ThreadLocal<CharsetEncoder> encoder = new ThreadLocalEncoder();
+
     /**
      * Flag that specifies if the connection should be considered closed. Closing a NIO connection
      * is an asynch operation so instead of waiting for the connection to be actually closed just
      * keep this flag to avoid using the connection between #close was used and the socket is actually
      * closed.
      */
-    private boolean closed;
+    private State state;
     
     /**
      * Lock used to ensure the integrity of the underlying IoSession (refer to
@@ -131,7 +134,7 @@ public class NIOConnection implements Connection {
     public NIOConnection(IoSession session, PacketDeliverer packetDeliverer) {
         this.ioSession = session;
         this.backupDeliverer = packetDeliverer;
-        closed = false;
+        state = State.RUNNING;
     }
 
     public boolean validate() {
@@ -216,26 +219,47 @@ public class NIOConnection implements Connection {
         return backupDeliverer;
     }
 
-    public void close() {
-    	synchronized(this) {
-    		if (isClosed()) {
-    			return;
-    		}
-            try {
-                deliverRawText(flashClient ? "</flash:stream>" : "</stream:stream>", true);
-            } catch (Exception e) {
-                // Ignore
+    public synchronized void close()
+    {
+        try
+        {
+            if ( state == State.CLOSED )
+            {
+                return;
+            }
+
+            // This prevents any action after the first invocation of close() on this connection.
+            if ( state != State.CLOSING )
+            {
+                state = State.CLOSING;
+                try
+                {
+                    deliverRawText( flashClient ? "</flash:stream>" : "</stream:stream>" );
+                }
+                catch ( Exception e )
+                {
+                    // Ignore
+                }
+            }
+
+            // deliverRawText might already have forced the state from Closing to Closed. In that case, there's no need
+            // to invoke the CloseListeners again.
+            if ( state == State.CLOSING )
+            {
+                // TODO Check for regression of OF-881 (which placed the call below outside of the synchronized block).
+                notifyCloseListeners(); // clean up session, etc.
             }
-            if (session != null) {
-                session.setStatus(Session.STATUS_CLOSED);
+        }
+        finally
+        {
+            // Ensure that the state of this connection, its session and the MINA context are eventually closed.
+            state = State.CLOSED;
+            if ( session != null )
+            {
+                session.setStatus( Session.STATUS_CLOSED );
             }
-            closed = true;
-    	}
-    	
-    	// OF-881: Notify any close listeners after the synchronized block has completed. 
-    	notifyCloseListeners(); // clean up session, etc.
-    	
-        ioSession.close(true); // sync via MINA
+            ioSession.close( true );
+        }
     }
 
     public void systemShutdown() {
@@ -263,7 +287,7 @@ public class NIOConnection implements Connection {
     }
 
     public synchronized boolean isClosed() {
-        return closed;
+        return state == State.CLOSED;
     }
 
     public boolean isSecure() {
@@ -324,12 +348,7 @@ public class NIOConnection implements Connection {
     }
 
     public void deliverRawText(String text) {
-        // Deliver the packet in asynchronous mode
-        deliverRawText(text, true);
-    }
-
-    private void deliverRawText(String text, boolean asynchronous) {
-        if (!isClosed()) {
+        if (state != State.CLOSED) {
             boolean errorDelivering = false;
             IoBuffer buffer = IoBuffer.allocate(text.length());
             buffer.setAutoExpand(true);
@@ -343,22 +362,12 @@ public class NIOConnection implements Connection {
                 buffer.flip();
                 ioSessionLock.lock();
                 try {
-                    if (asynchronous) {
-                        // OF-464: handle dropped connections (no backupDeliverer in this case?)
-                        if (!ioSession.isConnected()) {
-                            throw new IOException("Connection reset/closed by peer");
-                        }
-                        ioSession.write(buffer);
+                    // OF-464: handle dropped connections (no backupDeliverer in this case?)
+                    if (!ioSession.isConnected()) {
+                        throw new IOException("Connection reset/closed by peer");
                     }
-                    else {
-                        // Send stanza and wait for ACK (using a 2 seconds default timeout)
-                        boolean ok =
-                                ioSession.write(buffer).awaitUninterruptibly(JiveGlobals.getIntProperty("connection.ack.timeout", 2000));
-                        if (!ok) {
-                            Log.warn("No ACK was received when sending stanza to: " + this.toString());
-                        }
-                    }
-                } 
+                    ioSession.write(buffer);
+                }
                 finally {
                     ioSessionLock.unlock();
                 }
@@ -368,8 +377,8 @@ public class NIOConnection implements Connection {
                 errorDelivering = true;
             }
 
-            // Close the connection if delivering text fails and we are already not closing the connection
-            if (errorDelivering && asynchronous) {
+            // Attempt to close the connection if delivering text fails.
+            if (errorDelivering) {
                 close();
             }
         }