Prevent duplicate session destroy events (#12612)

Change-Id: Ic752268a9deac350dbff29ecf73cfce2eb1ba0cc

4 files changed, 99 insertions, 20 deletions

diff --git a/server/src/com/vaadin/server/VaadinService.java b/server/src/com/vaadin/server/VaadinService.java
index ba1224568a..b96e284e6e 100644
--- a/server/src/com/vaadin/server/VaadinService.java
+++ b/server/src/com/vaadin/server/VaadinService.java
@@ -53,6 +53,7 @@ import org.json.JSONObject;
 import com.vaadin.annotations.PreserveOnRefresh;
 import com.vaadin.event.EventRouter;
 import com.vaadin.server.VaadinSession.FutureAccess;
+import com.vaadin.server.VaadinSession.State;
 import com.vaadin.server.communication.FileUploadHandler;
 import com.vaadin.server.communication.HeartbeatHandler;
 import com.vaadin.server.communication.PublishedFileHandler;
@@ -446,7 +447,10 @@ public abstract class VaadinService implements Serializable {
         session.accessSynchronously(new Runnable() {
             @Override
             public void run() {
-                if (!session.isClosing()) {
+                if (session.getState() == State.CLOSED) {
+                    return;
+                }
+                if (session.getState() == State.OPEN) {
                     closeSession(session);
                 }
                 ArrayList<UI> uis = new ArrayList<UI>(session.getUIs());
@@ -472,6 +476,8 @@ public abstract class VaadinService implements Serializable {
                 // destroy listeners
                 eventRouter.fireEvent(new SessionDestroyEvent(
                         VaadinService.this, session), session.getErrorHandler());
+
+                session.setState(State.CLOSED);
             }
         });
     }
@@ -1127,7 +1133,7 @@ public abstract class VaadinService implements Serializable {
             closeInactiveUIs(session);
             removeClosedUIs(session);
         } else {
-            if (!session.isClosing()) {
+            if (session.getState() == State.OPEN) {
                 closeSession(session);
                 if (session.getSession() != null) {
                     getLogger().log(Level.FINE, "Closing inactive session {0}",
@@ -1279,7 +1285,7 @@ public abstract class VaadinService implements Serializable {
      * @return true if the session is active, false if it could be closed.
      */
     private boolean isSessionActive(VaadinSession session) {
-        if (session.isClosing() || session.getSession() == null) {
+        if (session.getState() != State.OPEN || session.getSession() == null) {
             return false;
         } else {
             long now = System.currentTimeMillis();
diff --git a/server/src/com/vaadin/server/VaadinSession.java b/server/src/com/vaadin/server/VaadinSession.java
index 134a026788..ac518c1902 100644
--- a/server/src/com/vaadin/server/VaadinSession.java
+++ b/server/src/com/vaadin/server/VaadinSession.java
@@ -167,6 +167,33 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
     }
 
     /**
+     * The lifecycle state of a VaadinSession.
+     * 
+     * @since 7.2
+     */
+    public enum State {
+        /**
+         * The session is active and accepting client requests.
+         */
+        OPEN,
+        /**
+         * The {@link VaadinSession#close() close} method has been called; the
+         * session will be closed as soon as the current request ends.
+         */
+        CLOSING,
+        /**
+         * The session is closed; all the {@link UI}s have been removed and
+         * {@link SessionDestroyListener}s have been called.
+         */
+        CLOSED;
+
+        private boolean isValidChange(State newState) {
+            return (this == OPEN && newState == CLOSING)
+                    || (this == CLOSING && newState == CLOSED);
+        }
+    }
+
+    /**
      * The name of the parameter that is by default used in e.g. web.xml to
      * define the name of the default {@link UI} class.
      */
@@ -225,7 +252,7 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
 
     private long lastRequestTimestamp = System.currentTimeMillis();
 
-    private boolean closing = false;
+    private State state = State.OPEN;
 
     private transient WrappedSession session;
 
@@ -279,24 +306,20 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
         } else if (VaadinService.getCurrentRequest() != null
                 && getCurrent() == this) {
             assert hasLock();
-            /*
-             * Ignore if the session is being moved to a different backing
-             * session or if GAEVaadinServlet is doing its normal cleanup.
-             */
+            // Ignore if the session is being moved to a different backing
+            // session or if GAEVaadinServlet is doing its normal cleanup.
             if (getAttribute(VaadinService.PRESERVE_UNBOUND_SESSION_ATTRIBUTE) == Boolean.TRUE) {
                 return;
             }
 
             // There is still a request in progress for this session. The
             // session will be destroyed after the response has been written.
-            if (!isClosing()) {
+            if (getState() == State.OPEN) {
                 close();
             }
         } else {
-            /*
-             * We are not in a request related to this session so we can
-             * immediately destroy it
-             */
+            // We are not in a request related to this session so we can destroy
+            // it as soon as we acquire the lock.
             service.fireSessionDestroy(this);
         }
         session = null;
@@ -1226,19 +1249,52 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
      */
     public void close() {
         assert hasLock();
-        closing = true;
+        state = State.CLOSING;
     }
 
     /**
-     * Returns whether this session is marked to be closed.
+     * Returns whether this session is marked to be closed. Note that this
+     * method also returns true if the session is actually already closed.
      * 
      * @see #close()
      * 
+     * @deprecated As of 7.2, use
+     *             <code>{@link #getState() getState() != State.OPEN}</code>
+     *             instead.
+     * 
      * @return true if this session is marked to be closed, false otherwise
      */
+    @Deprecated
     public boolean isClosing() {
         assert hasLock();
-        return closing;
+        return state == State.CLOSING || state == State.CLOSED;
+    }
+
+    /**
+     * Returns the lifecycle state of this session.
+     * 
+     * @since 7.2
+     * @return the current state
+     */
+    public State getState() {
+        assert hasLock();
+        return state;
+    }
+
+    /**
+     * Sets the lifecycle state of this session. The allowed transitions are
+     * OPEN to CLOSING and CLOSING to CLOSED.
+     * 
+     * @since 7.2
+     * @param state
+     *            the new state
+     */
+    protected void setState(State state) {
+        assert hasLock();
+        assert this.state.isValidChange(state) : "Invalid session state change "
+                + this.state + "->" + state;
+
+        this.state = state;
     }
 
     private static final Logger getLogger() {
diff --git a/server/src/com/vaadin/ui/UI.java b/server/src/com/vaadin/ui/UI.java
index 2b2e773601..275aeb4c79 100644
--- a/server/src/com/vaadin/ui/UI.java
+++ b/server/src/com/vaadin/ui/UI.java
@@ -53,6 +53,7 @@ import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinService;
 import com.vaadin.server.VaadinServlet;
 import com.vaadin.server.VaadinSession;
+import com.vaadin.server.VaadinSession.State;
 import com.vaadin.server.communication.PushConnection;
 import com.vaadin.shared.Connector;
 import com.vaadin.shared.EventId;
@@ -1162,7 +1163,7 @@ public abstract class UI extends AbstractSingleComponentContainer implements
     public void close() {
         closing = true;
 
-        boolean sessionExpired = (session == null || session.isClosing());
+        boolean sessionExpired = (session == null || session.getState() != State.OPEN);
         getRpcProxy(UIClientRpc.class).uiClosed(sessionExpired);
         if (getPushConnection() != null) {
             // Push the Rpc to the client. The connection will be closed when
diff --git a/server/tests/src/com/vaadin/server/VaadinServiceTest.java b/server/tests/src/com/vaadin/server/VaadinServiceTest.java
index cead5df79c..77eb155378 100644
--- a/server/tests/src/com/vaadin/server/VaadinServiceTest.java
+++ b/server/tests/src/com/vaadin/server/VaadinServiceTest.java
@@ -29,6 +29,16 @@ import org.junit.Test;
  */
 public class VaadinServiceTest {
 
+    private class TestSessionDestroyListener implements SessionDestroyListener {
+
+        int callCount = 0;
+
+        @Override
+        public void sessionDestroy(SessionDestroyEvent event) {
+            callCount++;
+        }
+    }
+
     @Test
     public void testFireSessionDestroy() throws ServletException {
         ServletConfig servletConfig = new MockServletConfig();
@@ -36,6 +46,10 @@ public class VaadinServiceTest {
         servlet.init(servletConfig);
         VaadinService service = servlet.getService();
 
+        TestSessionDestroyListener listener = new TestSessionDestroyListener();
+
+        service.addSessionDestroyListener(listener);
+
         MockVaadinSession vaadinSession = new MockVaadinSession(service);
         service.fireSessionDestroy(vaadinSession);
         Assert.assertEquals(
@@ -45,9 +59,11 @@ public class VaadinServiceTest {
         vaadinSession.valueUnbound(EasyMock
                 .createMock(HttpSessionBindingEvent.class));
 
-        org.junit.Assert.assertEquals(
-                "'fireSessionDestroy' method may not call 'close' "
-                        + "method for closing session", 1,
+        Assert.assertEquals("'fireSessionDestroy' method may not call 'close' "
+                + "method for closing session", 1,
                 vaadinSession.getCloseCount());
+
+        Assert.assertEquals("SessionDestroyListeners not called exactly once",
+                1, listener.callCount);
     }
 }