Fix handling of disconnection with push (#15330)

* The resource should not be closed when the client disconnects
* Removed "automatic" disconnection which was needed when onresume was not handled
* Don't call disconnect twice for cancelled connections

Change-Id: Id5ad9924b56c8810f759d6f9dc1da6e83e53d75b

2 files changed, 63 insertions, 30 deletions

diff --git a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java
index 0819a24ee9..aa952b9072 100644
--- a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java
+++ b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java
@@ -156,9 +156,9 @@ public class AtmospherePushConnection implements PushConnection {
     public void push(boolean async) {
         if (!isConnected()) {
             if (async && state != State.RESPONSE_PENDING) {
-                state = State.PUSH_PENDING;
+                setState(State.PUSH_PENDING);
             } else {
-                state = State.RESPONSE_PENDING;
+                setState(State.RESPONSE_PENDING);
             }
         } else {
             try {
@@ -229,23 +229,22 @@ public class AtmospherePushConnection implements PushConnection {
     /**
      * Associates this {@code AtmospherePushConnection} with the given
      * {@AtmosphereResource} representing an established
-     * push connection. If already connected, calls {@link #disconnect()} first.
+     * push connection.
+     * <p>
      * If there is a deferred push, carries it out via the new connection.
+     * <p>
+     * This method must never be called when there is an existing connection
      * 
      * @since 7.2
      */
     public void connect(AtmosphereResource resource) {
-
         assert resource != null;
         assert resource != this.resource;
-
-        if (isConnected()) {
-            disconnect();
-        }
+        assert !isConnected();
 
         this.resource = resource;
         State oldState = state;
-        state = State.CONNECTED;
+        setState(State.CONNECTED);
 
         if (oldState == State.PUSH_PENDING
                 || oldState == State.RESPONSE_PENDING) {
@@ -273,14 +272,17 @@ public class AtmospherePushConnection implements PushConnection {
     @Override
     public void disconnect() {
         assert isConnected();
-
-        if (resource.isResumed()) {
-            // Calling disconnect may end up invoking it again via
-            // resource.resume and PushHandler.onResume. Bail out here if
-            // the resource is already resumed; this is a bit hacky and should
-            // be implemented in a better way in 7.2.
-            resource = null;
-            state = State.DISCONNECTED;
+        if (!isConnected() || resource.isResumed()) {
+            // Server has asked to push connection to be disconnected but there
+            // is no current connection
+
+            // This could be a timing issue during session expiration or
+            // similar and should not happen in an ideal world. Might require
+            // some logic changes in session expiration handling to avoid
+            // triggering this
+            getLogger().info(
+                    "Server requested disconnect for inactive push connection");
+            connectionLost();
             return;
         }
 
@@ -302,13 +304,24 @@ public class AtmospherePushConnection implements PushConnection {
         }
 
         try {
+            // Close the push connection
             resource.close();
         } catch (IOException e) {
             getLogger()
                     .log(Level.INFO, "Error when closing push connection", e);
         }
+        connectionLost();
+    }
+
+    /**
+     * Called when the connection to the client has been lost.
+     * 
+     * @since
+     */
+    public void connectionLost() {
         resource = null;
-        state = State.DISCONNECTED;
+        setState(State.DISCONNECTED);
+
     }
 
     /**
@@ -319,6 +332,16 @@ public class AtmospherePushConnection implements PushConnection {
     }
 
     /**
+     * Sets the state of this connection
+     * 
+     * @param state
+     *            the new state
+     */
+    private void setState(State state) {
+        this.state = state;
+    }
+
+    /**
      * Reinitializes this PushConnection after deserialization. The connection
      * is initially in disconnected state; the client will handle the
      * reconnecting.
@@ -326,7 +349,7 @@ public class AtmospherePushConnection implements PushConnection {
     private void readObject(ObjectInputStream stream) throws IOException,
             ClassNotFoundException {
         stream.defaultReadObject();
-        state = State.DISCONNECTED;
+        setState(State.DISCONNECTED);
     }
 
     private static Logger getLogger() {
diff --git a/server/src/com/vaadin/server/communication/PushHandler.java b/server/src/com/vaadin/server/communication/PushHandler.java
index 7e7183193a..38a5f3017f 100644
--- a/server/src/com/vaadin/server/communication/PushHandler.java
+++ b/server/src/com/vaadin/server/communication/PushHandler.java
@@ -63,8 +63,9 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter {
         public void onStateChange(AtmosphereResourceEvent event)
                 throws IOException {
             super.onStateChange(event);
-            if (event.isCancelled() || event.isResumedOnTimeout()) {
-                disconnect(event);
+
+            if (event.isResumedOnTimeout()) {
+                connectionLost(event);
             }
         }
 
@@ -329,17 +330,30 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter {
     public void onDisconnect(AtmosphereResourceEvent event) {
         // Log event on trace level
         super.onDisconnect(event);
-        disconnect(event);
+
+        // Called no matter if the client closed the connection cleanly
+        // (event.isClosedByClient()) or if it was unexpectedly disconnected
+        // (event.isCancelled)
+        connectionLost(event);
+    }
+
+    @Override
+    public void onResume(AtmosphereResourceEvent event) {
+        super.onResume(event);
+
+        // If a long polling connection is resumed, we no longer have a
+        // connection up and must update the state of AtmospherePushConnection
+        connectionLost(event);
     }
 
     @Override
     public void onThrowable(AtmosphereResourceEvent event) {
         getLogger().log(Level.SEVERE, "Exception in push connection",
                 event.throwable());
-        disconnect(event);
+        connectionLost(event);
     }
 
-    private void disconnect(AtmosphereResourceEvent event) {
+    private void connectionLost(AtmosphereResourceEvent event) {
         // We don't want to use callWithUi here, as it assumes there's a client
         // request active and does requestStart and requestEnd among other
         // things.
@@ -425,12 +439,8 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter {
                                     "Connection unexpectedly closed for resource {0} with transport {1}",
                                     new Object[] { id, resource.transport() });
                 }
-                if (pushConnection.isConnected()) {
-                    // disconnect() assumes the push connection is connected but
-                    // this method can currently be called more than once during
-                    // disconnect, depending on the situation
-                    pushConnection.disconnect();
-                }
+
+                pushConnection.connectionLost();
             }
 
         } catch (final Exception e) {