OF-881: Apply review feedback

- Use AtomicReference instead of synchronized block
- Extend java.io.Closeable; document contract
- Consolidate close/state transition logic

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,7 +145,11 @@ public interface Connection {
      *      <li>Call notifyEvent all listeners that the channel is shutting down.
      *      <li>Close the socket.
      * </ul>
+     * 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();
 
     /**
@@ -435,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,40 +467,50 @@ public class SocketConnection implements Connection {
 
     @Override
     public void close() {
-    	
-        synchronized (this) {
-        	if (isClosed()) {
-        		return;
-        	}
+    	close(false);
+    }
+
+    /**
+     * 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);
             }
-        }
-        
-        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');
+
+            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();
+	            }
             }
-            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();
             
-        closeConnection();
-        notifyCloseListeners();
+    	}
     }
 
     @Override
@@ -537,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 {
@@ -550,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;
             }
         }
@@ -562,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,25 +192,21 @@ public abstract class VirtualConnection implements Connection {
      */
     @Override
     public void close() {
-    	
-        synchronized (this) {
-        	if (isClosed()) {
-        		return;
-        	}
+    	if (state.compareAndSet(State.OPEN, State.CLOSED)) {
+    		
             if (session != null) {
                 session.setStatus(Session.STATUS_CLOSED);
             }
+            
+            try {
+                closeVirtualConnection();
+            } catch (Exception e) {
+                Log.error(LocaleUtils.getLocalizedString("admin.error.close") + "\n" + toString(), e);
+            }
+            
+            notifyCloseListeners();
+            
         }
-        
-        try {
-            closeVirtualConnection();
-        } catch (Exception e) {
-            Log.error(LocaleUtils.getLocalizedString("admin.error.close")
-                    + "\n" + this.toString(), e);
-        }
-        
-        closed = true;
-        notifyCloseListeners();
     }
 
     @Override

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,33 +228,29 @@ public class NIOConnection implements Connection {
 
     @Override
     public void close() {
-    	synchronized ( this ) {
-        	// prevent recursion while closing
-        	if ( state == State.CLOSED || state == State.CLOSING) {
-        		return;
-        	}
-            state = State.CLOSING;
-        }
+    	if (state.compareAndSet(State.OPEN, State.CLOSED)) {
 
-        try {
-            deliverRawText( flashClient ? "</flash:stream>" : "</stream:stream>" );
-        } catch ( Exception e ) {
-            Log.error("Failed to deliver stream close tag: " + e.getMessage());
-        }
-        
-        // Ensure that the state of this connection, its session and the MINA context are eventually closed.
-        if ( session != null ) {
-            session.setStatus( Session.STATUS_CLOSED );
-        }
-        
-        try {
-        	ioSession.close( true );
-        } catch (Exception e) {
-            Log.error("Exception while closing MINA session", e);
-        }
+            // Ensure that the state of this connection, its session and the MINA context are eventually closed.
+
+    		if ( session != null ) {
+                session.setStatus( Session.STATUS_CLOSED );
+            }
+
+            try {
+                deliverRawText( flashClient ? "</flash:stream>" : "</stream:stream>" );
+            } catch ( Exception e ) {
+                Log.error("Failed to deliver stream close tag: " + e.getMessage());
+            }
+            
+            try {
+            	ioSession.close( true );
+            } catch (Exception e) {
+                Log.error("Exception while closing MINA session", e);
+            }
         
-        state = State.CLOSED;
-        notifyCloseListeners(); // clean up session, etc.
+            notifyCloseListeners(); // clean up session, etc.
+
+    	}
     }
 
     @Override
@@ -285,7 +281,7 @@ public class NIOConnection implements Connection {
 
     @Override
     public boolean isClosed() {
-        return state == State.CLOSED;
+    	return state.get() == State.CLOSED;
     }
 
     @Override
@@ -295,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 {
@@ -341,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);