Merge pull request #220 from guusdk/OF-883

Two additional fixes that relate to OF-883

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,32 +128,33 @@ 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 {
-        Log.warn("Closing session due to exception: " + session, cause);
+    public void exceptionCaught(IoSession session, Throwable cause) throws Exception {
+        Log.warn("Closing connection due to exception in session: " + session, cause);
 
         try {
             // OF-524: Determine stream:error message.
@@ -167,7 +168,10 @@ public abstract class ConnectionHandler extends IoHandlerAdapter {
             final Connection connection = (Connection) session.getAttribute( CONNECTION );
             connection.deliverRawText( error.toXML() );
         } finally {
-            session.close( false );
+            final Connection connection = (Connection) session.getAttribute( CONNECTION );
+            if (connection != null) {
+                connection.close();
+            }
         }
     }
 
@@ -188,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,53 @@ 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 void close()
+    {
+        boolean notifyClose = false;
+        synchronized ( this ) {
+            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 )
+                {
+                    notifyClose = true;
+                }
             }
-            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 );
+                }
+                ioSession.close( true );
             }
-            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
+        }
+        if (notifyClose)
+        {
+            notifyCloseListeners(); // clean up session, etc.
+        }
     }
 
     public void systemShutdown() {
@@ -263,7 +293,7 @@ public class NIOConnection implements Connection {
     }
 
     public synchronized boolean isClosed() {
-        return closed;
+        return state == State.CLOSED;
     }
 
     public boolean isSecure() {
@@ -324,12 +354,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 +368,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);
-                    }
-                    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());
-                        }
+                    // OF-464: handle dropped connections (no backupDeliverer in this case?)
+                    if (!ioSession.isConnected()) {
+                        throw new IOException("Connection reset/closed by peer");
                     }
-                } 
+                    ioSession.write(buffer);
+                }
                 finally {
                     ioSessionLock.unlock();
                 }
@@ -368,8 +383,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();
             }
         }