From 3aa137c3aff923b5e9f0b0714a595f29165e423a Mon Sep 17 00:00:00 2001 From: =?utf8?q?Johannes=20Dahlstr=C3=B6m?= Date: Thu, 22 Nov 2012 13:31:02 +0200 Subject: [PATCH] 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 --- .../server/AbstractCommunicationManager.java | 11 -- .../com/vaadin/server/LegacyApplication.java | 2 +- .../src/com/vaadin/server/VaadinPortlet.java | 2 +- .../src/com/vaadin/server/VaadinService.java | 152 +++++++++++++++++- .../src/com/vaadin/server/VaadinServlet.java | 8 +- .../src/com/vaadin/server/VaadinSession.java | 135 ++-------------- server/src/com/vaadin/ui/UI.java | 148 ++++++++--------- .../vaadin/tests/integration/ProxyTest.java | 4 +- .../tests/minitutorials/v7b2/CleanupUI.java | 22 +-- 9 files changed, 253 insertions(+), 231 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(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(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. + *

+ * 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. + *

+ * 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. + *

+ * 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; @@ -209,6 +207,16 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable { lastRequestTimestamp = timestamp; } + /** + * 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. *

@@ -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. - *

- * 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(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 - *

- * 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 @@ -81,42 +78,6 @@ import com.vaadin.util.ReflectTools; 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 event.getUI() within - * this method. - * - * @param event - * The close event that was fired. - */ - public void cleanup(CleanupEvent event); - } - /** * The application to which this UI belongs */ @@ -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 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 - * null application. + * Sets the session to which this UI is assigned. *

- * 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()}. *

* * @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() */ @@ -688,18 +640,6 @@ public abstract class UI extends AbstractSingleComponentContainer implements ClickListener.clickMethod); } - /** - * 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)} @@ -721,18 +661,6 @@ public abstract class UI extends AbstractSingleComponentContainer implements listener); } - /** - * 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.) + *

+ * 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. + *

+ * 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: + *

    + *
  • after it is explicitly {@link #close() closed}. + *
  • when its session is closed or expires + *
  • after three missed heartbeat requests. + *
+ *

+ * 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(); + } } diff --git a/uitest/src/com/vaadin/tests/integration/ProxyTest.java b/uitest/src/com/vaadin/tests/integration/ProxyTest.java index 97a4efe90e..36532307fb 100644 --- a/uitest/src/com/vaadin/tests/integration/ProxyTest.java +++ b/uitest/src/com/vaadin/tests/integration/ProxyTest.java @@ -64,9 +64,9 @@ public class ProxyTest extends AbstractTestUI { stopButton.setEnabled(false); startButton.setDisableOnClick(true); - addCleanupListener(new CleanupListener() { + addDetachListener(new DetachListener() { @Override - public void cleanup(CleanupEvent event) { + public void detach(DetachEvent event) { if (server != null && server.isRunning()) { try { server.stop(); diff --git a/uitest/src/com/vaadin/tests/minitutorials/v7b2/CleanupUI.java b/uitest/src/com/vaadin/tests/minitutorials/v7b2/CleanupUI.java index 9e89d65811..4e70748985 100644 --- a/uitest/src/com/vaadin/tests/minitutorials/v7b2/CleanupUI.java +++ b/uitest/src/com/vaadin/tests/minitutorials/v7b2/CleanupUI.java @@ -16,32 +16,34 @@ package com.vaadin.tests.minitutorials.v7b2; +import com.vaadin.server.ClientConnector.DetachListener; import com.vaadin.server.VaadinRequest; import com.vaadin.ui.UI; -public class CleanupUI extends UI implements UI.CleanupListener { +public class CleanupUI extends UI implements DetachListener { @Override protected void init(VaadinRequest request) { - addCleanupListener(new UI.CleanupListener() { + addDetachListener(new DetachListener() { @Override - public void cleanup(UI.CleanupEvent event) { + public void detach(DetachEvent event) { releaseSomeResources(); } }); // ... - addCleanupListener(this); + addDetachListener(this); + } + + @Override + public void detach(DetachEvent event) { + releaseMoreResources(); } private void releaseSomeResources() { // ... } - @Override - public void cleanup(UI.CleanupEvent event) { - // do cleanup - event.getUI(); - // or equivalent: - UI.getCurrent(); + private void releaseMoreResources() { + // ... } } -- 2.39.5