OF-453: Improve exception handling for BOSH session termination to ensure...

OF-453: Improve exception handling for BOSH session termination to ensure sessions are cleaned up properly

git-svn-id: http://svn.igniterealtime.org/svn/repos/openfire/trunk@13605 b35dd754-fafc-0310-a699-88a17e54d16e

src/java/org/jivesoftware/openfire/http/HttpConnection.java

@@ -35,8 +35,8 @@ import java.security.cert.X509Certificate;
  */
 public class HttpConnection {
 	
+    private static final String RESPONSE_BODY = "response-body";
     private static final String CONNECTION_CLOSED = "connection closed";
-    private static final String SUSPENDED = "org.eclipse.jetty.continuation.Suspended";
 
     private final long requestId;
     private final X509Certificate[] sslCertificates;
@@ -46,7 +46,6 @@ public class HttpConnection {
     private HttpSession session;
     private Continuation continuation;
     private boolean isClosed;
-    private boolean isDelivered = false;
 
     /**
      * Constructs an HTTP Connection.
@@ -96,10 +95,6 @@ public class HttpConnection {
         return isSecure;
     }
 
-    public boolean isDelivered() {
-        return isDelivered;
-    }
-
     /**
      * Delivers content to the client. The content should be valid XMPP wrapped inside of a body.
      * A <i>null</i> value for body indicates that the connection should be closed and the client
@@ -124,8 +119,8 @@ public class HttpConnection {
         if (body == null) {
             body = CONNECTION_CLOSED;
         }
-        if (continuation != null && continuation.isSuspended()) {
-            continuation.setAttribute("response-body", body);
+        if (isSuspended()) {
+            continuation.setAttribute(RESPONSE_BODY, body);
             continuation.resume();
             session.incrementServerPacketCount();
         }
@@ -200,21 +195,25 @@ public class HttpConnection {
     void setContinuation(Continuation continuation) {
         this.continuation = continuation;
     }
+    
+    public boolean isSuspended() {
+    	return continuation != null && continuation.isSuspended();
+    }
+    
+    public boolean isExpired() {
+    	return continuation != null && continuation.isExpired();
+    }
 
     private String waitForResponse() throws HttpBindTimeoutException {
         // we enter this method when we have no messages pending delivery
 		// when we resume a suspended continuation, or when we time out
-		if (!Boolean.TRUE.equals(continuation.getAttribute(SUSPENDED))) {
+		if (continuation.isInitial()) {
 		    continuation.setTimeout(session.getWait() * JiveConstants.SECOND);
             continuation.suspend();
-            continuation.setAttribute(SUSPENDED, Boolean.TRUE);
             continuation.undispatch();
-        }
-
-        if (continuation.isResumed()) {
-            String deliverable = (String) continuation.getAttribute("response-body");
+        } else if (continuation.isResumed()) {
             // This will occur when the hold attribute of a session has been exceeded.
-            this.isDelivered = true;
+            String deliverable = (String) continuation.getAttribute(RESPONSE_BODY);
             if (deliverable == null) {
                 throw new HttpBindTimeoutException();
             }
@@ -224,8 +223,13 @@ public class HttpConnection {
             return deliverable;
         }
 
-        this.isDelivered = true;
         throw new HttpBindTimeoutException("Request " + requestId + " exceeded response time from " +
                 "server of " + session.getWait() + " seconds.");
     }
+
+	@Override
+	public String toString() {
+		return (session != null ? session.toString() : "[Anonymous]")
+				+ " rid: " + this.getRequestId();
+	}
 }

src/java/org/jivesoftware/openfire/http/HttpSession.java

@@ -463,7 +463,7 @@ public class HttpSession extends LocalClientSession {
         	synchronized (connectionQueue) {
 	            for (HttpConnection connection : connectionQueue) {
 	                // The session is currently active, set the last activity to the current time.
-	                if (!connection.isClosed()) {
+	                if (!(connection.isClosed() || connection.isExpired())) {
 	                    lastActivity = System.currentTimeMillis();
 	                    break;
 	                }
@@ -571,31 +571,8 @@ public class HttpSession extends LocalClientSession {
 		this.lastResponseEmpty = lastResponseEmpty;
 	}
 
-	/**
-	 * @deprecated Doesn't make sense if we have multiple connections with the same rid in the queue.
-	 * 	Use {@link #consumeResponse(HttpConnection)} instead
-	 */
-    public String getResponse(long requestID) throws HttpBindException {
-    	synchronized (connectionQueue) {
-	        for (HttpConnection connection : connectionQueue) {
-	            if (connection.getRequestId() == requestID) {
-	                String response = getResponse(connection);
-	
-	                // connection needs to be removed after response is returned to maintain idempotence
-	                // otherwise if this method is called again, after 'waiting', the InternalError
-	                // will be thrown because the connection is no longer in the queue.
-	                connectionQueue.remove(connection);
-	                fireConnectionClosed(connection);
-	                return response;
-	            }
-	        }
-    	}
-        throw new InternalError("Could not locate connection: " + requestID);
-    }
-    
     /**
-     * Similar to {@link #getResponse(long)} but returns the response for a specific connection instance
-     * rather than looking up on the request id. This is because it is possible for there to be multiple
+     * Returns the response for a specific connection instance. It is possible for there to be multiple
      * connections in the queue for the same rid so we need to be careful that we are accessing the correct
      * connection.
      * <p><b>Note that this method also removes the connection from the internal connection queue.</b>
@@ -1003,38 +980,40 @@ public class HttpSession extends LocalClientSession {
         if (isClosed) { return; }
         isClosed = true;
 
-        // close connection(s) and deliver pending elements (if any)
-        synchronized (connectionQueue) {
-	        for (HttpConnection toClose : connectionQueue) {
-	            try {
-	            	if (!toClose.isClosed()) {
-	            		if (!pendingElements.isEmpty() && toClose.getRequestId() == lastRequestID + 1) {
-	            			synchronized(pendingElements) {
-		            			deliver(toClose, pendingElements);
-				                lastRequestID = toClose.getRequestId();
-				                pendingElements.clear();
+        try {
+	        // close connection(s) and deliver pending elements (if any)
+	        synchronized (connectionQueue) {
+		        for (HttpConnection toClose : connectionQueue) {
+		            try {
+		            	if (!toClose.isClosed()) {
+		            		if (!pendingElements.isEmpty() && toClose.getRequestId() == lastRequestID + 1) {
+		            			synchronized(pendingElements) {
+			            			deliver(toClose, pendingElements);
+					                lastRequestID = toClose.getRequestId();
+					                pendingElements.clear();
+		            			}
+	            			} else {
+	            				toClose.deliverBody(null);
 	            			}
-            			} else {
-            				toClose.deliverBody(null);
-            			}
-	            	}
-	            } catch (HttpConnectionClosedException e) {
-	            	/* ignore ... already closed */
-	            }
+		            	}
+		            } catch (HttpConnectionClosedException e) {
+		            	/* ignore ... already closed */
+		            }
+		        }
 	        }
-        }
-
-    	synchronized (pendingElements) {
-	        for (Deliverable deliverable : pendingElements) {
-	            failDelivery(deliverable.getPackets());
+	
+	    	synchronized (pendingElements) {
+		        for (Deliverable deliverable : pendingElements) {
+		            failDelivery(deliverable.getPackets());
+		        }
+		        pendingElements.clear();
 	        }
-	        pendingElements.clear();
-        }
-
-        for (SessionListener listener : listeners) {
-            listener.sessionClosed(this);
+        } finally { // ensure the session is removed from the session map
+	        for (SessionListener listener : listeners) {
+	            listener.sessionClosed(this);
+	        }
+	        this.listeners.clear();
         }
-        this.listeners.clear();
     }
 
     private void failDelivery(final Collection<Packet> packets) {

src/java/org/jivesoftware/openfire/http/HttpSessionManager.java

@@ -34,6 +34,7 @@ import org.dom4j.DocumentException;
 import org.dom4j.DocumentHelper;
 import org.dom4j.Element;
 import org.dom4j.QName;
+import org.eclipse.jetty.util.log.Log;
 import org.jivesoftware.openfire.SessionManager;
 import org.jivesoftware.openfire.StreamID;
 import org.jivesoftware.openfire.auth.UnauthorizedException;
@@ -391,7 +392,11 @@ public class HttpSessionManager {
             long currentTime = System.currentTimeMillis();
             for (HttpSession session : sessionMap.values()) {
                 long lastActive = currentTime - session.getLastActivity();
+                if (Log.isDebugEnabled()) {
+                	Log.debug("Session was last active " + lastActive + " ms ago: " + session.getAddress());
+                }
                 if (lastActive > session.getInactivityTimeout() * JiveConstants.SECOND) {
+                	Log.info("Closing idle session: " + session.getAddress());
                     session.close();
                 }
             }

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

@@ -177,12 +177,12 @@ public abstract class VirtualConnection implements Connection {
                         session.setStatus(Session.STATUS_CLOSED);
                     }
                     closeVirtualConnection();
-                    closed = true;
                 }
                 catch (Exception e) {
                     Log.error(LocaleUtils.getLocalizedString("admin.error.close")
                             + "\n" + this.toString(), e);
                 }
+                closed = true;
                 wasClosed = true;
             }
         }