Ensure session is locked during cleanup (#10569)

* At the same time ensures request timer handling is done while session is locked (#10108)

Change-Id: Ifc36e1ac66d02e25fe98616319c014137bd94c10

server/src/com/vaadin/server/VaadinService.java

@@ -1024,13 +1024,19 @@ public abstract class VaadinService implements Serializable {
      * 
      * @param session
      */
-    private void removeClosedUIs(VaadinSession session) {
-        for (UI ui : new ArrayList<UI>(session.getUIs())) {
-            if (ui.isClosing()) {
-                getLogger().log(Level.FINER, "Removing closed UI {0}",
-                        ui.getUIId());
-                session.removeUI(ui);
-            }
+    private void removeClosedUIs(final VaadinSession session) {
+        ArrayList<UI> uis = new ArrayList<UI>(session.getUIs());
+        for (final UI ui : uis) {
+            ui.runSafely(new Runnable() {
+                @Override
+                public void run() {
+                    if (ui.isClosing()) {
+                        getLogger().log(Level.FINER, "Removing closed UI {0}",
+                                ui.getUIId());
+                        session.removeUI(ui);
+                    }
+                }
+            });
         }
     }
 
@@ -1177,10 +1183,23 @@ public abstract class VaadinService implements Serializable {
     public void requestEnd(VaadinRequest request, VaadinResponse response,
             VaadinSession session) {
         if (session != null) {
-            cleanupSession(session);
-            long duration = (System.nanoTime() - (Long) request
+            final VaadinSession finalSession = session;
+
+            session.runSafely(new Runnable() {
+                @Override
+                public void run() {
+                    cleanupSession(finalSession);
+                }
+            });
+
+            final long duration = (System.nanoTime() - (Long) request
                     .getAttribute(REQUEST_START_TIME_ATTRIBUTE)) / 1000000;
-            session.setLastRequestDuration(duration);
+            session.runSafely(new Runnable() {
+                @Override
+                public void run() {
+                    finalSession.setLastRequestDuration(duration);
+                }
+            });
         }
         CurrentInstance.clearAll();
     }

server/tests/src/com/vaadin/server/VaadinSessionTest.java

@@ -25,30 +25,38 @@ import javax.servlet.http.HttpSessionBindingEvent;
 import junit.framework.Assert;
 
 import org.easymock.EasyMock;
+import org.junit.Before;
 import org.junit.Test;
 
 import com.vaadin.server.ClientConnector.DetachEvent;
 import com.vaadin.server.ClientConnector.DetachListener;
 import com.vaadin.ui.UI;
+import com.vaadin.util.CurrentInstance;
 
 public class VaadinSessionTest {
 
-    @Test
-    public void threadLocalsAfterUnderlyingSessionTimeout() {
-
-        final VaadinServlet mockServlet = new VaadinServlet() {
+    private VaadinSession session;
+    private VaadinServlet mockServlet;
+    private VaadinServletService mockService;
+    private HttpSession mockHttpSession;
+    private WrappedSession mockWrappedSession;
+    private VaadinServletRequest vaadinRequest;
+    private UI ui;
+
+    @Before
+    public void setup() {
+        mockServlet = new VaadinServlet() {
             @Override
             public String getServletName() {
                 return "mockServlet";
             };
         };
 
-        final VaadinServletService mockService = new VaadinServletService(
-                mockServlet, EasyMock.createMock(DeploymentConfiguration.class));
+        mockService = new VaadinServletService(mockServlet,
+                EasyMock.createMock(DeploymentConfiguration.class));
 
-        HttpSession mockHttpSession = EasyMock.createMock(HttpSession.class);
-        WrappedSession mockWrappedSession = new WrappedHttpSession(
-                mockHttpSession) {
+        mockHttpSession = EasyMock.createMock(HttpSession.class);
+        mockWrappedSession = new WrappedHttpSession(mockHttpSession) {
             final ReentrantLock lock = new ReentrantLock();
 
             @Override
@@ -60,10 +68,10 @@ public class VaadinSessionTest {
             }
         };
 
-        final VaadinSession session = new VaadinSession(mockService);
+        session = new VaadinSession(mockService);
         session.storeInSession(mockService, mockWrappedSession);
 
-        final UI ui = new UI() {
+        ui = new UI() {
             Page page = new Page(this) {
                 @Override
                 public void init(VaadinRequest request) {
@@ -79,7 +87,7 @@ public class VaadinSessionTest {
                 return page;
             }
         };
-        VaadinServletRequest vaadinRequest = new VaadinServletRequest(
+        vaadinRequest = new VaadinServletRequest(
                 EasyMock.createMock(HttpServletRequest.class), mockService) {
             @Override
             public String getParameter(String name) {
@@ -96,6 +104,11 @@ public class VaadinSessionTest {
         ui.setSession(session);
         session.addUI(ui);
 
+    }
+
+    @Test
+    public void threadLocalsAfterUnderlyingSessionTimeout() {
+
         final AtomicBoolean detachCalled = new AtomicBoolean(false);
         ui.addDetachListener(new DetachListener() {
             @Override
@@ -112,4 +125,24 @@ public class VaadinSessionTest {
         session.valueUnbound(EasyMock.createMock(HttpSessionBindingEvent.class));
         Assert.assertTrue(detachCalled.get());
     }
+
+    @Test
+    public void threadLocalsAfterSessionDestroy() {
+        final AtomicBoolean detachCalled = new AtomicBoolean(false);
+        ui.addDetachListener(new DetachListener() {
+            @Override
+            public void detach(DetachEvent event) {
+                detachCalled.set(true);
+                Assert.assertEquals(ui, UI.getCurrent());
+                Assert.assertEquals(ui.getPage(), Page.getCurrent());
+                Assert.assertEquals(session, VaadinSession.getCurrent());
+                Assert.assertEquals(mockService, VaadinService.getCurrent());
+                Assert.assertEquals(mockServlet, VaadinServlet.getCurrent());
+            }
+        });
+        CurrentInstance.clearAll();
+        session.close();
+        mockService.cleanupSession(session);
+        Assert.assertTrue(detachCalled.get());
+    }
 }

uitest/src/com/vaadin/tests/applicationcontext/CloseSession.java

@@ -18,13 +18,16 @@ package com.vaadin.tests.applicationcontext;
 
 import javax.servlet.http.HttpSession;
 
+import com.vaadin.server.Page;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinService;
+import com.vaadin.server.VaadinSession;
 import com.vaadin.server.WrappedHttpSession;
 import com.vaadin.tests.components.AbstractTestUI;
 import com.vaadin.tests.util.Log;
 import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.UI;
 
 public class CloseSession extends AbstractTestUI {
     private static final String OLD_HASH_PARAM = "oldHash";
@@ -158,6 +161,17 @@ public class CloseSession extends AbstractTestUI {
     @Override
     public void detach() {
         super.detach();
-        System.out.println("UI " + getUIId() + " detached");
+        log.log("Detach of " + this + " (" + getUIId() + ")");
+        boolean correctUI = (UI.getCurrent() == this);
+        boolean correctPage = (Page.getCurrent() == getPage());
+        boolean correctVaadinSession = (VaadinSession.getCurrent() == getSession());
+        boolean correctVaadinService = (VaadinService.getCurrent() == getSession()
+                .getService());
+        log.log("UI.current correct in detach: " + correctUI);
+        log.log("Page.current correct in detach: " + correctPage);
+        log.log("VaadinSession.current correct in detach: "
+                + correctVaadinSession);
+        log.log("VaadinService.current correct in detach: "
+                + correctVaadinService);
     }
 }