Deal correctly with session expired during opening of push connection (#11705)

* Added SessionExpiredHandler which any RequestHandler can implement to customize session expired handling
* The issue can be tested by creating a non-serializable UI (e.g. new Object() in a field) and restarting the server.

Change-Id: I3eb6bc56298e025bcde088af53ea656fb44e897b

7 files changed, 168 insertions, 87 deletions

diff --git a/server/src/com/vaadin/server/RequestHandler.java b/server/src/com/vaadin/server/RequestHandler.java
index 9c4f0ae6a4..e04b223904 100644
--- a/server/src/com/vaadin/server/RequestHandler.java
+++ b/server/src/com/vaadin/server/RequestHandler.java
@@ -22,23 +22,23 @@ import java.io.Serializable;
 import com.vaadin.ui.UI;
 
 /**
- * Handler for producing a response to non-UIDL requests. Handlers can be added
- * to service sessions using
- * {@link VaadinSession#addRequestHandler(RequestHandler)}
+ * Handler for producing a response to HTTP requests. Handlers can be either
+ * added on a {@link VaadinService service} level, common for all users, or on a
+ * {@link VaadinSession session} level for only a single user.
  */
 public interface RequestHandler extends Serializable {
 
     /**
-     * Handles a non-UIDL request. If a response is written, this method should
-     * return <code>true</code> to indicate that no more request handlers should
-     * be invoked for the request.
+     * Called when a request needs to be handled. If a response is written, this
+     * method should return <code>true</code> to indicate that no more request
+     * handlers should be invoked for the request.
      * <p>
      * Note that request handlers by default do not lock the session. If you are
      * using VaadinSession or anything inside the VaadinSession you must ensure
      * the session is locked. This can be done by extending
      * {@link SynchronizedRequestHandler} or by using
-     * {@link VaadinSession#runSafelyInContext(Runnable)} or
-     * {@link UI#runSafelyInContext(Runnable)}.
+     * {@link VaadinSession#runSafely(Runnable)} or
+     * {@link UI#runSafely(Runnable)}.
      * </p>
      * 
      * @param session
diff --git a/server/src/com/vaadin/server/SessionExpiredHandler.java b/server/src/com/vaadin/server/SessionExpiredHandler.java
new file mode 100644
index 0000000000..6a7896f3d1
--- /dev/null
+++ b/server/src/com/vaadin/server/SessionExpiredHandler.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright 2000-2013 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.server;
+
+import java.io.IOException;
+
+/**
+ * A specialized RequestHandler which is capable of sending session expiration
+ * messages to the user.
+ * 
+ * @since 7.1
+ * @author Vaadin Ltd
+ */
+public interface SessionExpiredHandler extends RequestHandler {
+
+    /**
+     * Called when the a session expiration has occured and a notification needs
+     * to be sent to the user. If a response is written, this method should
+     * return <code>true</code> to indicate that no more
+     * {@link SessionExpiredHandler} handlers should be invoked for the request.
+     * 
+     * @param request
+     *            The request to handle
+     * @param response
+     *            The response object to which a response can be written.
+     * @return true if a response has been written and no further request
+     *         handlers should be called, otherwise false
+     * @throws IOException
+     *             If an IO error occurred
+     * @since 7.1
+     */
+    boolean handleSessionExpired(VaadinRequest request, VaadinResponse response)
+            throws IOException;
+
+}
diff --git a/server/src/com/vaadin/server/VaadinService.java b/server/src/com/vaadin/server/VaadinService.java
index a4520e77e2..7fbc604488 100644
--- a/server/src/com/vaadin/server/VaadinService.java
+++ b/server/src/com/vaadin/server/VaadinService.java
@@ -1283,6 +1283,13 @@ public abstract class VaadinService implements Serializable {
     /**
      * Handles the incoming request and writes the response into the response
      * object. Uses {@link #getRequestHandlers()} for handling the request.
+     * <p>
+     * If a session expiration is detected during request handling then each
+     * {@link RequestHandler request handler} has an opportunity to handle the
+     * expiration event if it implements {@link SessionExpiredHandler}. If no
+     * request handler handles session expiration a default expiration message
+     * will be written.
+     * </p>
      * 
      * @param request
      *            The incoming request
@@ -1409,8 +1416,48 @@ public abstract class VaadinService implements Serializable {
      *             Thrown if there was any problem handling the expiration of
      *             the session
      */
-    protected abstract void handleSessionExpired(VaadinRequest request,
-            VaadinResponse response) throws ServiceException;
+    protected void handleSessionExpired(VaadinRequest request,
+            VaadinResponse response) throws ServiceException {
+        SystemMessages systemMessages = getSystemMessages(
+                ServletPortletHelper.findLocale(null, null, request), request);
+
+        for (RequestHandler handler : getRequestHandlers()) {
+            if (handler instanceof SessionExpiredHandler) {
+                try {
+                    if (((SessionExpiredHandler) handler).handleSessionExpired(
+                            request, response)) {
+                        return;
+                    }
+                } catch (IOException e) {
+                    throw new ServiceException(
+                            "Handling of session expired failed", e);
+                }
+            }
+        }
+
+        // No request handlers handled the request. Write a normal HTTP response
+
+        try {
+            // If there is a URL, try to redirect there
+            String sessionExpiredURL = systemMessages.getSessionExpiredURL();
+            if (sessionExpiredURL != null
+                    && (response instanceof VaadinServletResponse)) {
+                ((VaadinServletResponse) response)
+                        .sendRedirect(sessionExpiredURL);
+            } else {
+                /*
+                 * Session expired as a result of a standard http request and we
+                 * have nowhere to redirect. Reloading would likely cause an
+                 * endless loop. This can at least happen if refreshing a
+                 * resource when the session has expired.
+                 */
+                response.sendError(HttpServletResponse.SC_GONE,
+                        "Session expired");
+            }
+        } catch (IOException e) {
+            throw new ServiceException(e);
+        }
+    }
 
     /**
      * Creates a JSON message which, when sent to client as-is, will cause a
diff --git a/server/src/com/vaadin/server/VaadinServletService.java b/server/src/com/vaadin/server/VaadinServletService.java
index e7df223f89..3b39f17849 100644
--- a/server/src/com/vaadin/server/VaadinServletService.java
+++ b/server/src/com/vaadin/server/VaadinServletService.java
@@ -17,7 +17,6 @@
 package com.vaadin.server;
 
 import java.io.File;
-import java.io.IOException;
 import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -27,14 +26,12 @@ import java.util.logging.Logger;
 
 import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 
 import org.atmosphere.util.Version;
 
 import com.vaadin.server.communication.PushRequestHandler;
 import com.vaadin.server.communication.ServletBootstrapHandler;
 import com.vaadin.server.communication.ServletUIInitHandler;
-import com.vaadin.shared.JsonConstants;
 import com.vaadin.ui.UI;
 
 public class VaadinServletService extends VaadinService {
@@ -272,72 +269,6 @@ public class VaadinServletService extends VaadinService {
         return appId;
     }
 
-    /*
-     * (non-Javadoc)
-     * 
-     * @see
-     * com.vaadin.server.VaadinService#handleSessionExpired(com.vaadin.server
-     * .VaadinRequest, com.vaadin.server.VaadinResponse)
-     */
-    @Override
-    protected void handleSessionExpired(VaadinRequest request,
-            VaadinResponse response) throws ServiceException {
-        if (!(request instanceof VaadinServletRequest)) {
-            throw new ServiceException(new IllegalArgumentException(
-                    "handleSessionExpired called with a non-VaadinServletRequest: "
-                            + request.getClass().getName()));
-        }
-
-        VaadinServletRequest servletRequest = (VaadinServletRequest) request;
-        VaadinServletResponse servletResponse = (VaadinServletResponse) response;
-
-        try {
-            SystemMessages ci = getSystemMessages(
-                    ServletPortletHelper.findLocale(null, null, request),
-                    request);
-            if (ServletPortletHelper.isUIDLRequest(request)) {
-                /*
-                 * Invalidate session (weird to have session if we're saying
-                 * that it's expired)
-                 * 
-                 * Session must be invalidated before criticalNotification as it
-                 * commits the response.
-                 */
-                servletRequest.getSession().invalidate();
-
-                writeStringResponse(
-                        response,
-                        JsonConstants.JSON_CONTENT_TYPE,
-                        createCriticalNotificationJSON(
-                                ci.getSessionExpiredCaption(),
-                                ci.getSessionExpiredMessage(), null,
-                                ci.getSessionExpiredURL()));
-            } else if (ServletPortletHelper.isHeartbeatRequest(request)) {
-                response.sendError(HttpServletResponse.SC_GONE,
-                        "Session expired");
-            } else {
-                // 'plain' http req - e.g. browser reload;
-                // just go ahead redirect the browser
-                String sessionExpiredURL = ci.getSessionExpiredURL();
-                if (sessionExpiredURL != null) {
-                    servletResponse.sendRedirect(sessionExpiredURL);
-                } else {
-                    /*
-                     * Session expired as a result of a standard http request
-                     * and we have nowhere to redirect. Reloading would likely
-                     * cause an endless loop. This can at least happen if
-                     * refreshing a resource when the session has expired.
-                     */
-                    response.sendError(HttpServletResponse.SC_GONE,
-                            "Session expired");
-                }
-            }
-        } catch (IOException e) {
-            throw new ServiceException(e);
-        }
-
-    }
-
     private static final Logger getLogger() {
         return Logger.getLogger(VaadinServletService.class.getName());
     }
diff --git a/server/src/com/vaadin/server/communication/HeartbeatHandler.java b/server/src/com/vaadin/server/communication/HeartbeatHandler.java
index a547d32b30..16c21224ab 100644
--- a/server/src/com/vaadin/server/communication/HeartbeatHandler.java
+++ b/server/src/com/vaadin/server/communication/HeartbeatHandler.java
@@ -21,6 +21,7 @@ import java.io.IOException;
 import javax.servlet.http.HttpServletResponse;
 
 import com.vaadin.server.ServletPortletHelper;
+import com.vaadin.server.SessionExpiredHandler;
 import com.vaadin.server.SynchronizedRequestHandler;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinResponse;
@@ -39,7 +40,8 @@ import com.vaadin.ui.UI;
  * @author Vaadin Ltd
  * @since 7.1
  */
-public class HeartbeatHandler extends SynchronizedRequestHandler {
+public class HeartbeatHandler extends SynchronizedRequestHandler implements
+        SessionExpiredHandler {
 
     /**
      * Handles a heartbeat request for the given session. Reads the GET
@@ -67,4 +69,22 @@ public class HeartbeatHandler extends SynchronizedRequestHandler {
 
         return true;
     }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see
+     * com.vaadin.server.SessionExpiredHandler#handleSessionExpired(com.vaadin
+     * .server.VaadinRequest, com.vaadin.server.VaadinResponse)
+     */
+    @Override
+    public boolean handleSessionExpired(VaadinRequest request,
+            VaadinResponse response) throws IOException {
+        if (!ServletPortletHelper.isHeartbeatRequest(request)) {
+            return false;
+        }
+
+        response.sendError(HttpServletResponse.SC_GONE, "Session expired");
+        return true;
+    }
 }
diff --git a/server/src/com/vaadin/server/communication/PushHandler.java b/server/src/com/vaadin/server/communication/PushHandler.java
index c9d451509c..0b3401ec1d 100644
--- a/server/src/com/vaadin/server/communication/PushHandler.java
+++ b/server/src/com/vaadin/server/communication/PushHandler.java
@@ -31,7 +31,9 @@ import org.json.JSONException;
 
 import com.vaadin.server.LegacyCommunicationManager.InvalidUIDLSecurityKeyException;
 import com.vaadin.server.ServiceException;
+import com.vaadin.server.ServletPortletHelper;
 import com.vaadin.server.SessionExpiredException;
+import com.vaadin.server.SystemMessages;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinService;
 import com.vaadin.server.VaadinServletRequest;
@@ -227,12 +229,26 @@ public class PushHandler implements AtmosphereHandler {
             try {
                 session = service.findVaadinSession(vaadinRequest);
             } catch (ServiceException e) {
-                // TODO Auto-generated catch block
-                e.printStackTrace();
-                return;
+                getLogger().log(Level.SEVERE,
+                        "Could not get session. This should never happen", e);
             } catch (SessionExpiredException e) {
-                // TODO Auto-generated catch block
-                e.printStackTrace();
+                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);
+                }
                 return;
             }
 
diff --git a/server/src/com/vaadin/server/communication/PushRequestHandler.java b/server/src/com/vaadin/server/communication/PushRequestHandler.java
index 3540078f85..575ec4a86f 100644
--- a/server/src/com/vaadin/server/communication/PushRequestHandler.java
+++ b/server/src/com/vaadin/server/communication/PushRequestHandler.java
@@ -28,6 +28,7 @@ import org.atmosphere.cpr.AtmosphereResponse;
 import com.vaadin.server.RequestHandler;
 import com.vaadin.server.ServiceException;
 import com.vaadin.server.ServletPortletHelper;
+import com.vaadin.server.SessionExpiredHandler;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinResponse;
 import com.vaadin.server.VaadinServletRequest;
@@ -43,12 +44,14 @@ import com.vaadin.server.VaadinSession;
  * @author Vaadin Ltd
  * @since 7.1
  */
-public class PushRequestHandler implements RequestHandler {
+public class PushRequestHandler implements RequestHandler,
+        SessionExpiredHandler {
 
     private AtmosphereFramework atmosphere;
     private PushHandler pushHandler;
 
-    public PushRequestHandler(VaadinServletService service) throws ServiceException {
+    public PushRequestHandler(VaadinServletService service)
+            throws ServiceException {
 
         atmosphere = new AtmosphereFramework();
 
@@ -101,4 +104,20 @@ public class PushRequestHandler implements RequestHandler {
     public void destroy() {
         atmosphere.destroy();
     }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see
+     * com.vaadin.server.SessionExpiredHandler#handleSessionExpired(com.vaadin
+     * .server.VaadinRequest, com.vaadin.server.VaadinResponse)
+     */
+    @Override
+    public boolean handleSessionExpired(VaadinRequest request,
+            VaadinResponse response) throws IOException {
+        // Websockets request must be handled by accepting the websocket
+        // connection and then sending session expired so we let
+        // PushRequestHandler handle it
+        return handleRequest(null, request, response);
+    }
 }