Merge pull request #374 from tevans/OF-881

OF-881: Simplify connection close semantics

src/java/org/jivesoftware/openfire/Connection.java

@@ -20,20 +20,21 @@
 
 package org.jivesoftware.openfire;
 
+import java.io.Closeable;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+
 import org.jivesoftware.openfire.auth.UnauthorizedException;
 import org.jivesoftware.openfire.session.LocalSession;
 import org.xmpp.packet.Packet;
 
-import java.net.UnknownHostException;
-import java.security.cert.Certificate;
-
 /**
  * Represents a connection on the server.
  *
  * @author Iain Shigeoka
  */
-public interface Connection {
-
+public interface Connection extends Closeable {
+	
     /**
      * Verifies that the connection is still live. Typically this is done by
      * sending a whitespace character between packets.
@@ -144,29 +145,13 @@ public interface Connection {
      *      <li>Call notifyEvent all listeners that the channel is shutting down.
      *      <li>Close the socket.
      * </ul>
-     *
-     * An invocation of this method is equal to invoking {@link #close(boolean)} with a parameter
-     * that is false.
+     * Note this method overrides the base interface to suppress exceptions. However,
+     * it otherwise fulfills the requirements of the {@link Closeable#close()} contract
+     * (idempotent, try-with-resources, etc.)
      */
+    @Override
     public void close();
 
-    /**
-     * Close this session including associated socket connection. The order of
-     * events for closing the session is:
-     * <ul>
-     *      <li>Set closing flag to prevent redundant shutdowns.
-     *      <li>Call notifyEvent all listeners that the channel is shutting down.
-     *      <li>Close the socket.
-     * </ul>
-     *
-     * This method takes into account the connection state of the peer. Specifically,
-     * when the peer is known to be in a disconnected state, no data will be sent
-     * (otherwise, this method can trigger the delivery of an end-of-stream signal).
-     *
-     * @param peerIsKnownToBeDisconnected should be set to true when the peer is known to no longer be available.
-     */
-    public void close( boolean peerIsKnownToBeDisconnected );
-
     /**
      * Notification message indicating that the server is being shutdown. Implementors
      * should send a stream error whose condition is system-shutdown before closing
@@ -455,4 +440,10 @@ public interface Connection {
          */
         needed
     }
+
+    /**
+     * Used to specify operational status for the corresponding connection
+     */
+    enum State { OPEN, CLOSED }
+
 }

src/java/org/jivesoftware/openfire/net/SocketConnection.java

@@ -35,6 +35,7 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 import javax.net.ssl.SSLPeerUnverifiedException;
 
@@ -88,6 +89,7 @@ public class SocketConnection implements Connection {
 
     private Writer writer;
     private AtomicBoolean writing = new AtomicBoolean(false);
+    private AtomicReference<State> state = new AtomicReference<State>(State.OPEN);
 
     /**
      * Deliverer to use when the connection is closed or was closed when delivering
@@ -307,10 +309,7 @@ public class SocketConnection implements Connection {
 
     @Override
     public boolean isClosed() {
-        if (session == null) {
-            return socket.isClosed();
-        }
-        return session.getStatus() == Session.STATUS_CLOSED;
+    	return state.get() == State.CLOSED;
     }
 
     @Override
@@ -468,55 +467,50 @@ public class SocketConnection implements Connection {
 
     @Override
     public void close() {
-        close( false );
+    	close(false);
     }
 
-    @Override
-    public void close( boolean peerIsKnownToBeDisconnected ) {
-        boolean wasClosed = false;
-        synchronized (this) {
-            if (!isClosed()) {
-                try {
-                    if (session != null) {
-                        session.setStatus(Session.STATUS_CLOSED);
-                    }
-
-                    if ( !peerIsKnownToBeDisconnected ) {
-                        boolean allowedToWrite = false;
-                        try {
-                            requestWriting();
-                            allowedToWrite = true;
-                            // Register that we started sending data on the connection
-                            writeStarted();
-                            writer.write("</stream:stream>");
-                            if (flashClient) {
-                                writer.write('\0');
-                            }
-                            writer.flush();
-                        }
-                        catch (IOException e) {
-                            // Do nothing
-                        }
-                        finally {
-                            // Register that we finished sending data on the connection
-                            writeFinished();
-                            if (allowedToWrite) {
-                                releaseWriting();
-                            }
-                        }
-                    }
-                }
-                catch (Exception e) {
-                    Log.error(LocaleUtils.getLocalizedString("admin.error.close")
-                            + "\n" + this.toString(), e);
-                }
-                closeConnection();
-                wasClosed = true;
+    /**
+     * Normal connection close will attempt to write the stream end tag. Otherwise this method
+     * forces the connection closed immediately. This method will be called from {@link SocketSendingTracker} 
+     * when sending data over the socket has taken a long time and we need to close the socket, discard
+     * the connection and its session.
+     */
+    private void close(boolean force) {   	
+    	if (state.compareAndSet(State.OPEN, State.CLOSED)) {
+    		
+            if (session != null) {
+                session.setStatus(Session.STATUS_CLOSED);
             }
-        }
-        if (wasClosed) {
+
+            if (!force) {
+	            boolean allowedToWrite = false;
+	            try {
+	                requestWriting();
+	                allowedToWrite = true;
+	                // Register that we started sending data on the connection
+	                writeStarted();
+	                writer.write("</stream:stream>");
+	                if (flashClient) {
+	                    writer.write('\0');
+	                }
+	                writer.flush();
+	            }
+	            catch (Exception e) {
+	                Log.error("Failed to deliver stream close tag: " + e.getMessage());
+	            }
+	            
+	            // Register that we finished sending data on the connection
+	            writeFinished();
+	            if (allowedToWrite) {
+	                releaseWriting();
+	            }
+            }
+                
+            closeConnection();
             notifyCloseListeners();
-        }
+            
+    	}
     }
 
     @Override
@@ -552,7 +546,7 @@ public class SocketConnection implements Connection {
                 Log.debug("Closing connection: " + this + " that started sending data at: " +
                         new Date(writeTimestamp));
             }
-            forceClose();
+            close(true); // force
             return true;
         }
         else {
@@ -565,7 +559,7 @@ public class SocketConnection implements Connection {
                 if (Log.isDebugEnabled()) {
                     Log.debug("Closing connection that has been idle: " + this);
                 }
-                forceClose();
+                close(true); // force
                 return true;
             }
         }
@@ -577,24 +571,6 @@ public class SocketConnection implements Connection {
         instances.remove(this);
     }
 
-    /**
-     * Forces the connection to be closed immediately no matter if closing the socket takes
-     * a long time. This method should only be called from {@link SocketSendingTracker} when
-     * sending data over the socket has taken a long time and we need to close the socket, discard
-     * the connection and its session.
-     */
-    private void forceClose() {
-        if (session != null) {
-            // Set that the session is closed. This will prevent threads from trying to
-            // deliver packets to this session thus preventing future locks.
-            session.setStatus(Session.STATUS_CLOSED);
-        }
-        closeConnection();
-        // Notify the close listeners so that the SessionManager can send unavailable
-        // presences if required.
-        notifyCloseListeners();
-    }
-
     private void closeConnection() {
         release();
         try {

src/java/org/jivesoftware/openfire/net/VirtualConnection.java

@@ -23,6 +23,7 @@ package org.jivesoftware.openfire.net;
 import java.security.cert.Certificate;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.jivesoftware.openfire.Connection;
 import org.jivesoftware.openfire.ConnectionCloseListener;
@@ -52,7 +53,7 @@ public abstract class VirtualConnection implements Connection {
     final private Map<ConnectionCloseListener, Object> listeners =
             new HashMap<>();
 
-    private boolean closed = false;
+   private AtomicReference<State> state = new AtomicReference<State>(State.OPEN);
 
     @Override
     public String getLanguage() {
@@ -95,10 +96,7 @@ public abstract class VirtualConnection implements Connection {
 
     @Override
     public boolean isClosed() {
-        if (session == null) {
-            return closed;
-        }
-        return session.getStatus() == Session.STATUS_CLOSED;
+    	return state.get() == State.CLOSED;
     }
 
     @Override
@@ -194,30 +192,20 @@ public abstract class VirtualConnection implements Connection {
      */
     @Override
     public void close() {
-        close( false );
-    }
-
-    @Override
-    public void close(boolean peerIsKnownToBeDisconnected) {
-        boolean wasClosed = false;
-        synchronized (this) {
-            if (!isClosed()) {
-                try {
-                    if (session != null) {
-                        session.setStatus(Session.STATUS_CLOSED);
-                    }
-                    closeVirtualConnection();
-                }
-                catch (Exception e) {
-                    Log.error(LocaleUtils.getLocalizedString("admin.error.close")
-                            + "\n" + this.toString(), e);
-                }
-                closed = true;
-                wasClosed = true;
+    	if (state.compareAndSet(State.OPEN, State.CLOSED)) {
+    		
+            if (session != null) {
+                session.setStatus(Session.STATUS_CLOSED);
             }
-        }
-        if (wasClosed) {
+            
+            try {
+                closeVirtualConnection();
+            } catch (Exception e) {
+                Log.error(LocaleUtils.getLocalizedString("admin.error.close") + "\n" + toString(), e);
+            }
+            
             notifyCloseListeners();
+            
         }
     }
 

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

@@ -33,6 +33,7 @@ import java.nio.charset.CodingErrorAction;
 import java.nio.charset.StandardCharsets;
 import java.security.KeyStore;
 import java.security.cert.Certificate;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.ReentrantLock;
 
 import javax.net.ssl.SSLContext;
@@ -53,7 +54,9 @@ import org.jivesoftware.openfire.auth.UnauthorizedException;
 import org.jivesoftware.openfire.keystore.IdentityStoreConfig;
 import org.jivesoftware.openfire.keystore.Purpose;
 import org.jivesoftware.openfire.keystore.TrustStoreConfig;
-import org.jivesoftware.openfire.net.*;
+import org.jivesoftware.openfire.net.ClientTrustManager;
+import org.jivesoftware.openfire.net.SSLConfig;
+import org.jivesoftware.openfire.net.ServerTrustManager;
 import org.jivesoftware.openfire.session.ConnectionSettings;
 import org.jivesoftware.openfire.session.LocalSession;
 import org.jivesoftware.openfire.session.Session;
@@ -75,8 +78,6 @@ public class NIOConnection implements Connection {
 
 	private static final Logger Log = LoggerFactory.getLogger(NIOConnection.class);
 
-    public enum State { RUNNING, CLOSING, CLOSED }
-
     private LocalSession session;
     private IoSession ioSession;
 
@@ -111,7 +112,7 @@ public class NIOConnection implements Connection {
      * keep this flag to avoid using the connection between #close was used and the socket is actually
      * closed.
      */
-    private volatile State state;
+    private AtomicReference<State> state = new AtomicReference<State>(State.OPEN);
     
     /**
      * Lock used to ensure the integrity of the underlying IoSession (refer to
@@ -127,7 +128,6 @@ public class NIOConnection implements Connection {
     public NIOConnection(IoSession session, PacketDeliverer packetDeliverer) {
         this.ioSession = session;
         this.backupDeliverer = packetDeliverer;
-        state = State.RUNNING;
     }
 
     @Override
@@ -228,60 +228,29 @@ public class NIOConnection implements Connection {
 
     @Override
     public void close() {
-        close( false );
-    }
+    	if (state.compareAndSet(State.OPEN, State.CLOSED)) {
 
-    @Override
-    public void close( boolean peerIsKnownToBeDisconnected )
-    {
-        boolean notifyClose = false;
-        synchronized ( this ) {
-            try
-            {
-                if ( state == State.CLOSED )
-                {
-                    return;
-                }
+            // Ensure that the state of this connection, its session and the MINA context are eventually closed.
 
-                // This prevents any action after the first invocation of close() on this connection.
-                if ( state != State.CLOSING )
-                {
-                    state = State.CLOSING;
-                    if ( !peerIsKnownToBeDisconnected )
-                    {
-                        try
-                        {
-                            deliverRawText( flashClient ? "</flash:stream>" : "</stream:stream>" );
-                        }
-                        catch ( Exception e )
-                        {
-                            // Ignore
-                        }
-                    }
-                }
+    		if ( session != null ) {
+                session.setStatus( Session.STATUS_CLOSED );
+            }
 
-                // 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;
-                }
+            try {
+                deliverRawText( flashClient ? "</flash:stream>" : "</stream:stream>" );
+            } catch ( Exception e ) {
+                Log.error("Failed to deliver stream close tag: " + e.getMessage());
             }
-            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 );
+            
+            try {
+            	ioSession.close( true );
+            } catch (Exception e) {
+                Log.error("Exception while closing MINA session", e);
             }
-        }
-        if (notifyClose)
-        {
+        
             notifyCloseListeners(); // clean up session, etc.
-        }
+
+    	}
     }
 
     @Override
@@ -312,7 +281,7 @@ public class NIOConnection implements Connection {
 
     @Override
     public boolean isClosed() {
-        return state == State.CLOSED;
+    	return state.get() == State.CLOSED;
     }
 
     @Override
@@ -322,7 +291,7 @@ public class NIOConnection implements Connection {
 
     @Override
     public void deliver(Packet packet) throws UnauthorizedException {
-        if (state != State.RUNNING) {
+        if (isClosed()) {
         	backupDeliverer.deliver(packet);
         }
         else {
@@ -368,7 +337,7 @@ public class NIOConnection implements Connection {
 
     @Override
     public void deliverRawText(String text) {
-        if (state != State.CLOSED) {
+        if (!isClosed()) {
             boolean errorDelivering = false;
             IoBuffer buffer = IoBuffer.allocate(text.length());
             buffer.setAutoExpand(true);