Refactor heartbeat handling and inactive UI/session closing (#10252)

* Package-private VaadinService.cleanupSession() handles these, called from VaadinServlet and VaadinPortlet at the end of a request
* UI detach() called when removing from the session (#9755)
* UIs can be explicitly closed; UIs marked as closed are removed at the end of the request (#10249)
* Remove UI cleanup events and listeners in favor of detach events (#10251)

Change-Id: I1f994c43bd2fc5fe7f99f7152c9db35927235291

7 files changed, 239 insertions, 219 deletions

diff --git a/server/src/com/vaadin/server/AbstractCommunicationManager.java b/server/src/com/vaadin/server/AbstractCommunicationManager.java
index e514524545..0832a1a33a 100644
--- a/server/src/com/vaadin/server/AbstractCommunicationManager.java
+++ b/server/src/com/vaadin/server/AbstractCommunicationManager.java
@@ -82,7 +82,6 @@ import com.vaadin.ui.Component;
 import com.vaadin.ui.ConnectorTracker;
 import com.vaadin.ui.HasComponents;
 import com.vaadin.ui.LegacyComponent;
-import com.vaadin.ui.LegacyWindow;
 import com.vaadin.ui.SelectiveRenderer;
 import com.vaadin.ui.UI;
 import com.vaadin.ui.Window;
@@ -642,16 +641,6 @@ public abstract class AbstractCommunicationManager implements Serializable {
      * 
      */
     protected void postPaint(UI uI) {
-        if (uI instanceof LegacyWindow) {
-            LegacyWindow legacyWindow = (LegacyWindow) uI;
-            if (!legacyWindow.getApplication().isRunning()) {
-                // Detach LegacyWindow if it belongs to a closed
-                // LegacyApplication
-                legacyWindow.setApplication(null);
-                legacyWindow.setSession(null);
-            }
-        }
-
         // Remove connectors that have been detached from the session during
         // handling of the request
         uI.getConnectorTracker().cleanConnectorMap();
diff --git a/server/src/com/vaadin/server/LegacyApplication.java b/server/src/com/vaadin/server/LegacyApplication.java
index d5e3f28587..a62c8e1492 100644
--- a/server/src/com/vaadin/server/LegacyApplication.java
+++ b/server/src/com/vaadin/server/LegacyApplication.java
@@ -230,7 +230,7 @@ public abstract class LegacyApplication implements ErrorHandler {
             if (logoutUrl != null) {
                 legacyWindow.getPage().setLocation(logoutUrl);
             }
-            legacyWindow.getSession().cleanupUI(legacyWindow);
+            legacyWindow.close();
         }
     }
 
diff --git a/server/src/com/vaadin/server/VaadinPortlet.java b/server/src/com/vaadin/server/VaadinPortlet.java
index cae3642592..58f7da0401 100644
--- a/server/src/com/vaadin/server/VaadinPortlet.java
+++ b/server/src/com/vaadin/server/VaadinPortlet.java
@@ -499,7 +499,7 @@ public class VaadinPortlet extends GenericPortlet implements Constants {
                             vaadinSession, e);
                 } finally {
                     if (vaadinSession != null) {
-                        vaadinSession.cleanupInactiveUIs();
+                        getService().cleanupSession(vaadinSession);
                         requestTimer.stop(vaadinSession);
                     }
                 }
diff --git a/server/src/com/vaadin/server/VaadinService.java b/server/src/com/vaadin/server/VaadinService.java
index eaa5b51b30..ea363dc9cf 100644
--- a/server/src/com/vaadin/server/VaadinService.java
+++ b/server/src/com/vaadin/server/VaadinService.java
@@ -25,6 +25,7 @@ import java.net.URL;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Locale;
+import java.util.logging.Logger;
 
 import javax.portlet.PortletContext;
 import javax.servlet.ServletContext;
@@ -334,7 +335,13 @@ public abstract class VaadinService implements Serializable {
         }
 
         for (UI ui : new ArrayList<UI>(vaadinSession.getUIs())) {
-            vaadinSession.cleanupUI(ui);
+            // close() called here for consistency so that it is always called
+            // before a UI is removed. UI.isClosing() is thus always true in
+            // UI.detach() and associated detach listeners.
+            if (!ui.isClosing()) {
+                ui.close();
+            }
+            vaadinSession.removeUI(ui);
         }
 
         eventRouter.fireEvent(new SessionDestroyEvent(this, vaadinSession));
@@ -764,4 +771,147 @@ public abstract class VaadinService implements Serializable {
     public void closeSession(VaadinSession session) {
         session.removeFromSession(this);
     }
+
+    /**
+     * Called at the end of a request, after sending the response. Closes
+     * inactive UIs in the given session, removes closed UIs from the session,
+     * and closes the session if it is itself inactive.
+     * 
+     * @param session
+     */
+    void cleanupSession(VaadinSession session) {
+        if (isSessionActive(session)) {
+            closeInactiveUIs(session);
+            removeClosedUIs(session);
+        } else {
+            getLogger().fine(
+                    "Closed inactive session " + session.getSession().getId());
+            closeSession(session);
+        }
+    }
+
+    /**
+     * Removes those UIs from the given session for which {@link UI#isClosing()
+     * isClosing} yields true.
+     * 
+     * @param session
+     */
+    private void removeClosedUIs(VaadinSession session) {
+        for (UI ui : new ArrayList<UI>(session.getUIs())) {
+            if (ui.isClosing()) {
+                getLogger().finer("Removing closed UI " + ui.getUIId());
+                session.removeUI(ui);
+            }
+        }
+    }
+
+    /**
+     * Closes those UIs in the given session for which {@link #isUIActive}
+     * yields false.
+     * 
+     * @since 7.0.0
+     */
+    private void closeInactiveUIs(VaadinSession session) {
+        String sessionId = session.getSession().getId();
+        for (UI ui : session.getUIs()) {
+            if (!isUIActive(ui) && !ui.isClosing()) {
+                getLogger().fine(
+                        "Closing inactive UI #" + ui.getUIId() + " in session "
+                                + sessionId);
+                ui.close();
+            }
+        }
+    }
+
+    /**
+     * Returns the number of seconds that must pass without a valid heartbeat or
+     * UIDL request being received from a UI before that UI is removed from its
+     * session. This is a lower bound; it might take longer to close an inactive
+     * UI. Returns a negative number if heartbeat is disabled and timeout never
+     * occurs.
+     * 
+     * @see DeploymentConfiguration#getHeartbeatInterval()
+     * 
+     * @since 7.0.0
+     * 
+     * @return The heartbeat timeout in seconds or a negative number if timeout
+     *         never occurs.
+     */
+    private int getHeartbeatTimeout() {
+        // Permit three missed heartbeats before closing the UI
+        return (int) (getDeploymentConfiguration().getHeartbeatInterval() * (3.1));
+    }
+
+    /**
+     * Returns the number of seconds that must pass without a valid UIDL request
+     * being received for the given session before the session is closed, even
+     * though heartbeat requests are received. This is a lower bound; it might
+     * take longer to close an inactive session.
+     * <p>
+     * Returns a negative number if there is no timeout. In this case heartbeat
+     * requests suffice to keep the session alive, but it will still eventually
+     * expire in the regular manner if there are no requests at all (see
+     * {@link WrappedSession#getMaxInactiveInterval()}).
+     * 
+     * @see DeploymentConfiguration#isCloseIdleSessions()
+     * @see #getHeartbeatTimeout()
+     * 
+     * @since 7.0.0
+     * 
+     * @return The UIDL request timeout in seconds, or a negative number if
+     *         timeout never occurs.
+     */
+    private int getUidlRequestTimeout(VaadinSession session) {
+        return getDeploymentConfiguration().isCloseIdleSessions() ? session
+                .getSession().getMaxInactiveInterval() : -1;
+    }
+
+    /**
+     * Returns whether the given UI is active (the client-side actively
+     * communicates with the server) or whether it can be removed from the
+     * session and eventually collected.
+     * <p>
+     * A UI is active if and only if its {@link UI#isClosing() isClosing}
+     * returns false and {@link #getHeartbeatTimeout() getHeartbeatTimeout} is
+     * negative or has not yet expired.
+     * 
+     * @since 7.0.0
+     * 
+     * @param ui
+     *            The UI whose status to check
+     * 
+     * @return true if the UI is active, false if it could be removed.
+     */
+    private boolean isUIActive(UI ui) {
+        if (ui.isClosing()) {
+            return false;
+        } else {
+            long now = System.currentTimeMillis();
+            int timeout = 1000 * getHeartbeatTimeout();
+            return timeout < 0
+                    || now - ui.getLastHeartbeatTimestamp() < timeout;
+        }
+    }
+
+    /**
+     * Returns whether the given session is active or whether it can be closed.
+     * <p>
+     * A session is always active if
+     * {@link #getUidlRequestTimeout(VaadinSession) getUidlRequestTimeout} is
+     * negative. Otherwise, it is active if and only if the timeout has not
+     * expired.
+     * 
+     * @param session
+     *            The session whose status to check
+     * @return true if the session is active, false if it could be closed.
+     */
+    private boolean isSessionActive(VaadinSession session) {
+        long now = System.currentTimeMillis();
+        int timeout = 1000 * getUidlRequestTimeout(session);
+        return timeout < 0 || now - session.getLastRequestTimestamp() < timeout;
+    }
+
+    private static final Logger getLogger() {
+        return Logger.getLogger(VaadinService.class.getName());
+    }
 }
diff --git a/server/src/com/vaadin/server/VaadinServlet.java b/server/src/com/vaadin/server/VaadinServlet.java
index e0c3390ba5..9b2eba04db 100644
--- a/server/src/com/vaadin/server/VaadinServlet.java
+++ b/server/src/com/vaadin/server/VaadinServlet.java
@@ -343,14 +343,10 @@ public class VaadinServlet extends HttpServlet implements Constants {
             handleServiceException(request, response, vaadinSession, e);
         } finally {
             if (vaadinSession != null) {
-                vaadinSession.cleanupInactiveUIs();
-            }
-
-            CurrentInstance.clearAll();
-
-            if (vaadinSession != null) {
+                getService().cleanupSession(vaadinSession);
                 requestTimer.stop(vaadinSession);
             }
+            CurrentInstance.clearAll();
         }
     }
 
diff --git a/server/src/com/vaadin/server/VaadinSession.java b/server/src/com/vaadin/server/VaadinSession.java
index 1e5c4ec516..3ebdb94248 100644
--- a/server/src/com/vaadin/server/VaadinSession.java
+++ b/server/src/com/vaadin/server/VaadinSession.java
@@ -18,7 +18,6 @@ package com.vaadin.server;
 
 import java.io.Serializable;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -28,7 +27,6 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
-import java.util.logging.Logger;
 
 import javax.portlet.PortletSession;
 import javax.servlet.http.HttpSession;
@@ -210,6 +208,16 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
     }
 
     /**
+     * Returns the time when the last request was serviced in this session.
+     * 
+     * @return The time when the last request was handled, in milliseconds since
+     *         the epoch.
+     */
+    public long getLastRequestTimestamp() {
+        return lastRequestTimestamp;
+    }
+
+    /**
      * Gets the underlying session to which this service session is currently
      * associated.
      * 
@@ -513,10 +521,6 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
         return String.valueOf(connectorIdSequence++);
     }
 
-    private static final Logger getLogger() {
-        return Logger.getLogger(VaadinSession.class.getName());
-    }
-
     /**
      * Returns a UI with the given id.
      * <p>
@@ -583,126 +587,17 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
     }
 
     /**
-     * Removes all those UIs from the session for which {@link #isUIAlive}
-     * returns false. Cleanup events are fired for the removed UIs.
-     * <p>
-     * Called by the framework at the end of every request.
-     * 
-     * @see UI.CleanupEvent
-     * @see UI.CleanupListener
-     * @see #isUIAlive(UI)
-     * 
-     * @since 7.0.0
-     * 
-     * @deprecated As of 7.0. Will likely change or be removed in a future
-     *             version
-     */
-    @Deprecated
-    public void cleanupInactiveUIs() {
-        if (getUidlRequestTimeout() >= 0
-                && System.currentTimeMillis() - lastRequestTimestamp > 1000 * getUidlRequestTimeout()) {
-            close();
-        } else {
-            for (UI ui : new ArrayList<UI>(uIs.values())) {
-                if (!isUIAlive(ui)) {
-                    cleanupUI(ui);
-                    getLogger()
-                            .fine("Closed UI #" + ui.getUIId()
-                                    + " due to inactivity");
-                }
-            }
-        }
-    }
-
-    /**
-     * Called by the framework to remove an UI instance because it has been
-     * inactive.
+     * Called by the framework to remove an UI instance from the session because
+     * it has been closed.
      * 
      * @param ui
      *            the UI to remove
-     * 
-     * @deprecated As of 7.0. Method is declared as public only to support
-     *             {@link LegacyApplication#close()} and will be removed when
-     *             LegacyApplciation support is removed.
      */
-    @Deprecated
-    public void cleanupUI(UI ui) {
-        Integer id = Integer.valueOf(ui.getUIId());
+    public void removeUI(UI ui) {
+        int id = ui.getUIId();
+        ui.setSession(null);
         uIs.remove(id);
         retainOnRefreshUIs.values().remove(id);
-        ui.fireCleanupEvent();
-    }
-
-    /**
-     * Returns the number of seconds that must pass without a valid heartbeat or
-     * UIDL request being received from a UI before that UI is removed from the
-     * application. This is a lower bound; it might take longer to close an
-     * inactive UI. Returns a negative number if heartbeat is disabled and
-     * timeout never occurs.
-     * 
-     * @see #getUidlRequestTimeout()
-     * @see #cleanupInactiveUIs()
-     * @see DeploymentConfiguration#getHeartbeatInterval()
-     * 
-     * @since 7.0.0
-     * 
-     * @return The heartbeat timeout in seconds or a negative number if timeout
-     *         never occurs.
-     */
-    protected int getHeartbeatTimeout() {
-        // Permit three missed heartbeats before closing the UI
-        return (int) (configuration.getHeartbeatInterval() * (3.1));
-    }
-
-    /**
-     * Returns the number of seconds that must pass without a valid UIDL request
-     * being received from a UI before the UI is removed from the session, even
-     * though heartbeat requests are received. This is a lower bound; it might
-     * take longer to close an inactive UI. Returns a negative number if
-     * <p>
-     * This timeout only has effect if cleanup of inactive UIs is enabled;
-     * otherwise heartbeat requests are enough to extend UI lifetime
-     * indefinitely.
-     * 
-     * @see DeploymentConfiguration#isCloseIdleSessions()
-     * @see #getHeartbeatTimeout()
-     * @see #cleanupInactiveUIs()
-     * 
-     * @since 7.0.0
-     * 
-     * @return The UIDL request timeout in seconds, or a negative number if
-     *         timeout never occurs.
-     */
-    protected int getUidlRequestTimeout() {
-        return configuration.isCloseIdleSessions() ? getSession()
-                .getMaxInactiveInterval() : -1;
-    }
-
-    /**
-     * Returns whether the given UI is alive (the client-side actively
-     * communicates with the server) or whether it can be removed from the
-     * session and eventually collected.
-     * 
-     * @since 7.0.0
-     * 
-     * @deprecated As of 7.0. Will likely change or be removed in a future
-     *             version
-     * 
-     * @param ui
-     *            The UI whose status to check
-     * @return true if the UI is alive, false if it could be removed.
-     * 
-     * @deprecated As of 7.0. Will likely change or be removed in a future
-     *             version
-     */
-    @Deprecated
-    protected boolean isUIAlive(UI ui) {
-        long now = System.currentTimeMillis();
-        if (getHeartbeatTimeout() >= 0
-                && now - ui.getLastHeartbeatTimestamp() > 1000 * getHeartbeatTimeout()) {
-            return false;
-        }
-        return true;
     }
 
     /**
diff --git a/server/src/com/vaadin/ui/UI.java b/server/src/com/vaadin/ui/UI.java
index 2887c71adf..e99846c84b 100644
--- a/server/src/com/vaadin/ui/UI.java
+++ b/server/src/com/vaadin/ui/UI.java
@@ -16,7 +16,6 @@
 
 package com.vaadin.ui;
 
-import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -27,7 +26,6 @@ import java.util.Map;
 import com.vaadin.event.Action;
 import com.vaadin.event.Action.Handler;
 import com.vaadin.event.ActionManager;
-import com.vaadin.event.ConnectorEventListener;
 import com.vaadin.event.MouseEvents.ClickEvent;
 import com.vaadin.event.MouseEvents.ClickListener;
 import com.vaadin.navigator.Navigator;
@@ -45,7 +43,6 @@ import com.vaadin.shared.ui.ui.UIConstants;
 import com.vaadin.shared.ui.ui.UIServerRpc;
 import com.vaadin.shared.ui.ui.UIState;
 import com.vaadin.util.CurrentInstance;
-import com.vaadin.util.ReflectTools;
 
 /**
  * The topmost component in any component hierarchy. There is one UI for every
@@ -82,42 +79,6 @@ public abstract class UI extends AbstractSingleComponentContainer implements
         Action.Container, Action.Notifier, LegacyComponent {
 
     /**
-     * Event fired when a UI is removed from the application.
-     */
-    public static class CleanupEvent extends Event {
-
-        private static final String CLEANUP_EVENT_IDENTIFIER = "uiCleanup";
-
-        public CleanupEvent(UI source) {
-            super(source);
-        }
-
-        public UI getUI() {
-            return (UI) getSource();
-        }
-    }
-
-    /**
-     * Interface for listening {@link UI.CleanupEvent UI cleanup events}.
-     * 
-     */
-    public interface CleanupListener extends ConnectorEventListener {
-
-        public static final Method cleanupMethod = ReflectTools.findMethod(
-                CleanupListener.class, "cleanup", CleanupEvent.class);
-
-        /**
-         * Called when a CleanupListener is notified of a CleanupEvent.
-         * {@link UI#getCurrent()} returns <code>event.getUI()</code> within
-         * this method.
-         * 
-         * @param event
-         *            The close event that was fired.
-         */
-        public void cleanup(CleanupEvent event);
-    }
-
-    /**
      * The application to which this UI belongs
      */
     private VaadinSession session;
@@ -174,6 +135,8 @@ public abstract class UI extends AbstractSingleComponentContainer implements
      */
     private long lastHeartbeatTimestamp = System.currentTimeMillis();
 
+    private boolean closing = false;
+
     /**
      * Creates a new empty UI without a caption. The content of the UI must be
      * set by calling {@link #setContent(Component)} before using the UI.
@@ -286,16 +249,6 @@ public abstract class UI extends AbstractSingleComponentContainer implements
         fireEvent(new ClickEvent(this, mouseDetails));
     }
 
-    /**
-     * For internal use only.
-     */
-    public void fireCleanupEvent() {
-        UI current = UI.getCurrent();
-        UI.setCurrent(this);
-        fireEvent(new CleanupEvent(this));
-        UI.setCurrent(current);
-    }
-
     @Override
     @SuppressWarnings("unchecked")
     public void changeVariables(Object source, Map<String, Object> variables) {
@@ -347,18 +300,17 @@ public abstract class UI extends AbstractSingleComponentContainer implements
     }
 
     /**
-     * Sets the application to which this UI is assigned. It is not legal to
-     * change the application once it has been set nor to set a
-     * <code>null</code> application.
+     * Sets the session to which this UI is assigned.
      * <p>
-     * This method is mainly intended for internal use by the framework.
+     * This method is for internal use by the framework. To explicitly close a
+     * UI, see {@link #close()}.
      * </p>
      * 
      * @param session
-     *            the application to set
+     *            the session to set
      * 
      * @throws IllegalStateException
-     *             if the application has already been set
+     *             if the session has already been set
      * 
      * @see #getSession()
      */
@@ -689,18 +641,6 @@ public abstract class UI extends AbstractSingleComponentContainer implements
     }
 
     /**
-     * Adds a cleanup listener to the UI. Cleanup listeners are invoked when the
-     * UI is removed from the session due to UI or session expiration.
-     * 
-     * @param listener
-     *            The CleanupListener that should be added.
-     */
-    public void addCleanupListener(CleanupListener listener) {
-        addListener(CleanupEvent.CLEANUP_EVENT_IDENTIFIER, CleanupEvent.class,
-                listener, CleanupListener.cleanupMethod);
-    }
-
-    /**
      * @deprecated As of 7.0, replaced by
      *             {@link #addClickListener(ClickListener)}
      **/
@@ -722,18 +662,6 @@ public abstract class UI extends AbstractSingleComponentContainer implements
     }
 
     /**
-     * Removes a cleanup listener from the UI, if previously added with
-     * {@link #addCleanupListener(CleanupListener)}.
-     * 
-     * @param listener
-     *            The CleanupListener that should be removed
-     */
-    public void removeCleanupListener(CleanupListener listener) {
-        removeListener(CleanupEvent.CLEANUP_EVENT_IDENTIFIER,
-                CleanupEvent.class, listener);
-    }
-
-    /**
      * @deprecated As of 7.0, replaced by
      *             {@link #removeClickListener(ClickListener)}
      **/
@@ -979,4 +907,66 @@ public abstract class UI extends AbstractSingleComponentContainer implements
     public String getTheme() {
         return theme;
     }
+
+    /**
+     * Marks this UI to be {@link #detach() detached} from the session at the
+     * end of the current request, or the next request if there is no current
+     * request (if called from a background thread, for instance.)
+     * <p>
+     * The UI is detached after the response is sent, so in the current request
+     * it can still update the client side normally. However, after the response
+     * any new requests from the client side to this UI will cause an error, so
+     * usually the client should be asked, for instance, to reload the page
+     * (serving a fresh UI instance), to close the page, or to navigate
+     * somewhere else.
+     * <p>
+     * Note that this method is strictly for users to explicitly signal the
+     * framework that the UI should be detached. Overriding it is not a reliable
+     * way to catch UIs that are to be detached. Instead, {@code UI.detach()}
+     * should be overridden or a {@link DetachListener} used.
+     */
+    public void close() {
+        closing = true;
+    }
+
+    /**
+     * Returns whether this UI is marked as closed and is to be detached.
+     * 
+     * @see #close()
+     * 
+     * @return whether this UI is closing.
+     */
+    public boolean isClosing() {
+        return closing;
+    }
+
+    /**
+     * Called after the UI is added to the session. A UI instance is attached
+     * exactly once, before its {@link #init(VaadinRequest) init} method is
+     * called.
+     * 
+     * @see Component#attach
+     */
+    @Override
+    public void attach() {
+        super.attach();
+    }
+
+    /**
+     * Called before the UI is removed from the session. A UI instance is
+     * detached exactly once, either:
+     * <ul>
+     * <li>after it is explicitly {@link #close() closed}.
+     * <li>when its session is closed or expires
+     * <li>after three missed heartbeat requests.
+     * </ul>
+     * <p>
+     * Note that when a UI is detached, any changes made in the {@code detach}
+     * methods of any children or {@link DetachListener}s that would be
+     * communicated to the client are silently ignored.
+     */
+    @Override
+    public void detach() {
+        super.detach();
+    }
 }