Ensure PushConnection is properly cleaned up on disconnect (#12226, #12522)

Change-Id: I0bab199632554655ef92a624f5654852b4b157d1

4 files changed, 272 insertions, 36 deletions

diff --git a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java
index e325550c4b..b9d4955b12 100644
--- a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java
+++ b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java
@@ -221,8 +221,6 @@ public class AtmospherePushConnection implements PushConnection {
         }
 
         resource.resume();
-        assert !resource.getBroadcaster().getAtmosphereResources()
-                .contains(resource);
         resource = null;
     }
 
diff --git a/server/src/com/vaadin/server/communication/PushHandler.java b/server/src/com/vaadin/server/communication/PushHandler.java
index 1c50f79349..81dd00084d 100644
--- a/server/src/com/vaadin/server/communication/PushHandler.java
+++ b/server/src/com/vaadin/server/communication/PushHandler.java
@@ -173,7 +173,8 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
     };
 
     /**
-     * Callback used when a connection is closed by the client.
+     * Callback used when a connection is closed, either deliberately or because
+     * an error occurred.
      */
     private final PushEventCallback disconnectCallback = new PushEventCallback() {
         @Override
@@ -192,8 +193,7 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
                 if (!pushMode.isEnabled()) {
                     /*
                      * The client is expected to close the connection after push
-                     * mode has been set to disabled, just clean up some stuff
-                     * and be done with it
+                     * mode has been set to disabled.
                      */
                     getLogger().log(Level.FINER,
                             "Connection closed for resource {0}", id);
@@ -248,24 +248,17 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
             } catch (ServiceException e) {
                 getLogger().log(Level.SEVERE,
                         "Could not get session. This should never happen", e);
+                return;
             } catch (SessionExpiredException e) {
                 SystemMessages msg = service.getSystemMessages(
                         ServletPortletHelper.findLocale(null, null,
                                 vaadinRequest), vaadinRequest);
-                try {
-                    resource.getResponse()
-                            .getWriter()
-                            .write(VaadinService
-                                    .createCriticalNotificationJSON(
-                                            msg.getSessionExpiredCaption(),
-                                            msg.getSessionExpiredMessage(),
-                                            null, msg.getSessionExpiredURL()));
-                } catch (IOException e1) {
-                    getLogger()
-                            .log(Level.WARNING,
-                                    "Failed to notify client about unavailable session",
-                                    e);
-                }
+                sendNotificationAndDisconnect(
+                        resource,
+                        VaadinService.createCriticalNotificationJSON(
+                                msg.getSessionExpiredCaption(),
+                                msg.getSessionExpiredMessage(), null,
+                                msg.getSessionExpiredURL()));
                 return;
             }
 
@@ -275,21 +268,15 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
                 // Sets UI.currentInstance
                 final UI ui = service.findUI(vaadinRequest);
                 if (ui == null) {
-                    // This a request through an already open push connection to
-                    // a UI which no longer exists.
-                    resource.getResponse()
-                            .getWriter()
-                            .write(UidlRequestHandler.getUINotFoundErrorJSON(
-                                    service, vaadinRequest));
-                    // End the connection
-                    resource.resume();
-                    return;
+                    sendNotificationAndDisconnect(resource,
+                            UidlRequestHandler.getUINotFoundErrorJSON(service,
+                                    vaadinRequest));
+                } else {
+                    callback.run(resource, ui);
                 }
-
-                callback.run(resource, ui);
             } catch (IOException e) {
-                getLogger().log(Level.INFO,
-                        "An error occured while writing a push response", e);
+                getLogger().log(Level.WARNING, "Error writing a push response",
+                        e);
             } finally {
                 session.unlock();
             }
@@ -323,9 +310,10 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
         AtmosphereResource resource = event.getResource();
 
         String id = resource.uuid();
-        if (event.isCancelled()) {
-            // Disconnected for whatever reason, handle in onDisconnect() as
-            // it's more reliable
+        if (event.isCancelled() || event.isResumedOnTimeout()) {
+            getLogger().log(Level.FINER,
+                    "Cancelled connection for resource {0}", id);
+            disconnect(event);
         } else if (event.isResuming()) {
             // A connection that was suspended earlier was resumed (committed to
             // the client.) Should only happen if the transport is JSONP or
@@ -364,13 +352,31 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
     public void onDisconnect(AtmosphereResourceEvent event) {
         // Log event on trace level
         super.onDisconnect(event);
-        callWithUi(event.getResource(), disconnectCallback);
+        disconnect(event);
+    }
+
+    @Override
+    public void onThrowable(AtmosphereResourceEvent event) {
+        getLogger().log(Level.SEVERE, "Exception in push connection",
+                event.throwable());
+        disconnect(event);
+    }
+
+    @Override
+    public void onResume(AtmosphereResourceEvent event) {
+        // Log event on trace level
+        super.onResume(event);
+        disconnect(event);
     }
 
     @Override
     public void destroy() {
     }
 
+    private void disconnect(AtmosphereResourceEvent event) {
+        callWithUi(event.getResource(), disconnectCallback);
+    }
+
     /**
      * Sends a refresh message to the given atmosphere resource. Uses an
      * AtmosphereResource instead of an AtmospherePushConnection even though it
@@ -396,6 +402,22 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter
         }
     }
 
+    /**
+     * Tries to send a critical notification to the client and close the
+     * connection. Does nothing if the connection is already closed.
+     */
+    private static void sendNotificationAndDisconnect(
+            AtmosphereResource resource, String notificationJson) {
+        // TODO Implemented differently from sendRefreshAndDisconnect
+        try {
+            resource.getResponse().getWriter().write(notificationJson);
+            resource.resume();
+        } catch (Exception e) {
+            getLogger().log(Level.FINEST,
+                    "Failed to send critical notification to client", e);
+        }
+    }
+
     private static final Logger getLogger() {
         return Logger.getLogger(PushHandler.class.getName());
     }
diff --git a/uitest/src/com/vaadin/tests/push/EnableDisablePush.html b/uitest/src/com/vaadin/tests/push/EnableDisablePush.html
new file mode 100644
index 0000000000..87dfa8428f
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/push/EnableDisablePush.html
@@ -0,0 +1,97 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
+<head profile="http://selenium-ide.openqa.org/profiles/test-case">
+<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
+<link rel="selenium.base" href="" />
+<title>EnableDisablePush</title>
+</head>
+<body>
+<table cellpadding="1" cellspacing="1" border="1">
+<thead>
+<tr><td rowspan="1" colspan="3">EnableDisablePush</td></tr>
+</thead><tbody>
+<tr>
+	<td>open</td>
+	<td>/run/EnableDisablePush?restartApplication</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>1. Push enabled</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>3. Push disabled</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>5. Poll enabled</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[1]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>7. Push enabled</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[2]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>9. Poll disabled</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[4]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>11. Push disabled, polling enabled</td>
+</tr>
+<tr>
+	<td>pause</td>
+	<td>3500</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>16. Polling disabled, push enabled</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runEnableDisablePush::PID_SLog_row_0</td>
+	<td>18. Push disabled</td>
+</tr>
+
+</tbody></table>
+</body>
+</html>
diff --git a/uitest/src/com/vaadin/tests/push/EnableDisablePush.java b/uitest/src/com/vaadin/tests/push/EnableDisablePush.java
new file mode 100644
index 0000000000..1911c66c2d
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/push/EnableDisablePush.java
@@ -0,0 +1,119 @@
+package com.vaadin.tests.push;
+
+import java.util.Date;
+import java.util.Timer;
+import java.util.TimerTask;
+import java.util.concurrent.TimeUnit;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.communication.PushMode;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.tests.util.Log;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.UIDetachedException;
+
+public class EnableDisablePush extends AbstractTestUI {
+
+    private int c = 0;
+
+    private Log log = new Log(15);
+
+    private final Timer timer = new Timer(true);
+
+    private final class CounterTask extends TimerTask {
+
+        @Override
+        public void run() {
+
+            try {
+                while (true) {
+                    TimeUnit.MILLISECONDS.sleep(1000);
+
+                    access(new Runnable() {
+                        @Override
+                        public void run() {
+                            log.log("Counter = " + c++);
+                            if (c == 3) {
+                                log.log("Disabling polling, enabling push");
+                                getPushConfiguration().setPushMode(
+                                        PushMode.AUTOMATIC);
+                                setPollInterval(-1);
+                                log.log("Polling disabled, push enabled");
+                            }
+                        }
+                    });
+                }
+            } catch (InterruptedException e) {
+            } catch (UIDetachedException e) {
+            }
+        }
+    };
+
+    @Override
+    protected void setup(VaadinRequest request) {
+
+        getPushConfiguration().setPushMode(PushMode.AUTOMATIC);
+        log.log("Push enabled");
+
+        addComponent(new Button("Disable push", new Button.ClickListener() {
+            @Override
+            public void buttonClick(Button.ClickEvent event) {
+                log.log("Disabling push");
+                getPushConfiguration().setPushMode(PushMode.DISABLED);
+                log.log("Push disabled");
+            }
+        }));
+
+        addComponent(new Button("Enable push", new Button.ClickListener() {
+            @Override
+            public void buttonClick(Button.ClickEvent event) {
+                log.log("Enabling push");
+                getPushConfiguration().setPushMode(PushMode.AUTOMATIC);
+                log.log("Push enabled");
+            }
+        }));
+
+        addComponent(new Button("Disable polling", new Button.ClickListener() {
+            @Override
+            public void buttonClick(Button.ClickEvent event) {
+                log.log("Disabling poll");
+                setPollInterval(-1);
+                log.log("Poll disabled");
+            }
+        }));
+
+        addComponent(new Button("Enable polling", new Button.ClickListener() {
+            @Override
+            public void buttonClick(Button.ClickEvent event) {
+                log.log("Enabling poll");
+                setPollInterval(1000);
+                log.log("Poll enabled");
+            }
+        }));
+
+        addComponent(new Button(
+                "Disable push, re-enable from background thread",
+                new Button.ClickListener() {
+                    @Override
+                    public void buttonClick(Button.ClickEvent event) {
+                        log.log("Disabling push, enabling polling");
+                        getPushConfiguration().setPushMode(PushMode.DISABLED);
+                        setPollInterval(1000);
+                        timer.schedule(new CounterTask(), new Date());
+                        log.log("Push disabled, polling enabled");
+                    }
+                }));
+
+        addComponent(log);
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Test dynamically enablind and disabling push";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 12226;
+    }
+}