Define how CurrentInstance works with access() (#11897)

Change-Id: I7ca62c5706fd37e7c44ed46703bcdce159b367f4

3 files changed, 104 insertions, 35 deletions

diff --git a/server/src/com/vaadin/server/VaadinSession.java b/server/src/com/vaadin/server/VaadinSession.java
index 9625a3f350..eaf611caad 100644
--- a/server/src/com/vaadin/server/VaadinSession.java
+++ b/server/src/com/vaadin/server/VaadinSession.java
@@ -67,6 +67,35 @@ import com.vaadin.util.ReflectTools;
 @SuppressWarnings("serial")
 public class VaadinSession implements HttpSessionBindingListener, Serializable {
 
+    private class FutureAccess extends FutureTask<Void> {
+        /**
+         * Snapshot of all non-inheritable current instances at the time this
+         * object was created.
+         */
+        private final Map<Class<?>, CurrentInstance> instances = CurrentInstance
+                .getInstances(true);
+
+        public FutureAccess(Runnable arg0) {
+            super(arg0, null);
+        }
+
+        @Override
+        public Void get() throws InterruptedException, ExecutionException {
+            /*
+             * Help the developer avoid programming patterns that cause
+             * deadlocks unless implemented very carefully. get(long, TimeUnit)
+             * does not have the same detection since a sensible timeout should
+             * avoid completely locking up the application.
+             * 
+             * Even though no deadlock could occur after the runnable has been
+             * run, the check is always done as the deterministic behavior makes
+             * it easier to detect potential problems.
+             */
+            VaadinService.verifyNoOtherSessionLocked(VaadinSession.this);
+            return super.get();
+        }
+    }
+
     /**
      * 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.
@@ -140,7 +169,7 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
      * session is serialized as long as it doesn't happen while some other
      * thread has the lock.
      */
-    private transient final ConcurrentLinkedQueue<FutureTask<Void>> pendingAccessQueue = new ConcurrentLinkedQueue<FutureTask<Void>>();
+    private transient final ConcurrentLinkedQueue<FutureAccess> pendingAccessQueue = new ConcurrentLinkedQueue<FutureAccess>();
 
     /**
      * Create a new service session tied to a Vaadin service
@@ -1152,9 +1181,13 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
      * <p>
      * Please note that the runnable might be invoked on a different thread or
      * later on the current thread, which means that custom thread locals might
-     * not have the expected values when the runnable is executed. The session
-     * and other thread locals provided by Vaadin are set properly before
-     * executing the runnable.
+     * not have the expected values when the runnable is executed. Inheritable
+     * values in {@link CurrentInstance} will have the same values as when this
+     * method was invoked. {@link VaadinSession#getCurrent()} and
+     * {@link VaadinService#getCurrent()} are set according to this session
+     * before executing the runnable. Non-inheritable CurrentInstance values
+     * including {@link VaadinService#getCurrentRequest()} and
+     * {@link VaadinService#getCurrentResponse()} will not be defined.
      * </p>
      * <p>
      * The returned future can be used to check for task completion and to
@@ -1176,23 +1209,7 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
      *         cancel the task
      */
     public Future<Void> access(Runnable runnable) {
-        FutureTask<Void> future = new FutureTask<Void>(runnable, null) {
-            @Override
-            public Void get() throws InterruptedException, ExecutionException {
-                /*
-                 * Help the developer avoid programming patterns that cause
-                 * deadlocks unless implemented very carefully. get(long,
-                 * TimeUnit) does not have the same detection since a sensible
-                 * timeout should avoid completely locking up the application.
-                 * 
-                 * Even though no deadlock could occur after the runnable has
-                 * been run, the check is always done as the deterministic
-                 * behavior makes it easier to detect potential problems.
-                 */
-                VaadinService.verifyNoOtherSessionLocked(VaadinSession.this);
-                return super.get();
-            }
-        };
+        FutureAccess future = new FutureAccess(runnable);
         pendingAccessQueue.add(future);
 
         /*
@@ -1233,11 +1250,26 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
     public void runPendingAccessTasks() {
         assert hasLock();
 
-        FutureTask<Void> pendingAccess;
-        while ((pendingAccess = pendingAccessQueue.poll()) != null) {
-            if (!pendingAccess.isCancelled()) {
-                accessSynchronously(pendingAccess);
+        if (pendingAccessQueue.isEmpty()) {
+            return;
+        }
+
+        Map<Class<?>, CurrentInstance> oldInstances = CurrentInstance
+                .getInstances(false);
+
+        FutureAccess pendingAccess;
+        try {
+            while ((pendingAccess = pendingAccessQueue.poll()) != null) {
+                if (!pendingAccess.isCancelled()) {
+                    CurrentInstance.clearAll();
+                    CurrentInstance
+                            .restoreThreadLocals(pendingAccess.instances);
+                    accessSynchronously(pendingAccess);
+                }
             }
+        } finally {
+            CurrentInstance.clearAll();
+            CurrentInstance.restoreThreadLocals(oldInstances);
         }
     }
 
diff --git a/server/src/com/vaadin/ui/UI.java b/server/src/com/vaadin/ui/UI.java
index bbc1d0cd33..0b21112c97 100644
--- a/server/src/com/vaadin/ui/UI.java
+++ b/server/src/com/vaadin/ui/UI.java
@@ -1185,9 +1185,14 @@ public abstract class UI extends AbstractSingleComponentContainer implements
      * <p>
      * Please note that the runnable might be invoked on a different thread or
      * later on the current thread, which means that custom thread locals might
-     * not have the expected values when the runnable is executed. The UI and
-     * other thread locals provided by Vaadin are set properly before executing
-     * the runnable.
+     * not have the expected values when the runnable is executed. Inheritable
+     * values in {@link CurrentInstance} will have the same values as when this
+     * method was invoked. {@link UI#getCurrent()},
+     * {@link VaadinSession#getCurrent()} and {@link VaadinService#getCurrent()}
+     * are set according to this UI before executing the runnable.
+     * Non-inheritable CurrentInstance values including
+     * {@link VaadinService#getCurrentRequest()} and
+     * {@link VaadinService#getCurrentResponse()} will not be defined.
      * </p>
      * <p>
      * The returned future can be used to check for task completion and to
diff --git a/server/src/com/vaadin/util/CurrentInstance.java b/server/src/com/vaadin/util/CurrentInstance.java
index 0854d422fd..8e6ef8266e 100644
--- a/server/src/com/vaadin/util/CurrentInstance.java
+++ b/server/src/com/vaadin/util/CurrentInstance.java
@@ -17,33 +17,35 @@
 package com.vaadin.util;
 
 import java.io.Serializable;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
 
-import com.vaadin.server.VaadinPortlet;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinResponse;
 import com.vaadin.server.VaadinService;
-import com.vaadin.server.VaadinServlet;
 import com.vaadin.server.VaadinSession;
 import com.vaadin.ui.UI;
 
 /**
- * Keeps track of various thread local instances used by the framework.
+ * Keeps track of various current instances for the current thread. All the
+ * instances are automatically cleared after handling a request from the client
+ * to avoid leaking memory. The inheritable values are also maintained when
+ * execution is moved to another thread, both when a new thread is created and
+ * when {@link VaadinSession#access(Runnable)} or {@link UI#access(Runnable)} is
+ * used.
  * <p>
  * Currently the framework uses the following instances:
  * </p>
  * <p>
- * Inheritable: {@link UI}, {@link VaadinPortlet}, {@link VaadinService},
- * {@link VaadinServlet}, {@link VaadinSession}.
+ * Inheritable: {@link UI}, {@link VaadinService}, {@link VaadinSession}.
  * </p>
  * <p>
  * Non-inheritable: {@link VaadinRequest}, {@link VaadinResponse}.
  * </p>
  * 
  * @author Vaadin Ltd
- * @version @VERSION@
  * @since 7.0.0
  */
 public class CurrentInstance implements Serializable {
@@ -115,7 +117,9 @@ public class CurrentInstance implements Serializable {
 
     /**
      * Sets the current inheritable instance of the given type. A current
-     * instance that is inheritable will be available for child threads.
+     * instance that is inheritable will be available for child threads and in
+     * code run by {@link VaadinSession#access(Runnable)} and
+     * {@link UI#access(Runnable)}.
      * 
      * @see #set(Class, Object)
      * @see InheritableThreadLocal
@@ -184,6 +188,34 @@ public class CurrentInstance implements Serializable {
     }
 
     /**
+     * Gets the currently set instances so that they can later be restored using
+     * {@link #restoreThreadLocals(Map)}.
+     * 
+     * @since 7.1
+     * 
+     * @param onlyInheritable
+     *            <code>true</code> if only the inheritable instances should be
+     *            included; <code>false</code> to get all instances.
+     * @return a map containing the current instances
+     */
+    public static Map<Class<?>, CurrentInstance> getInstances(
+            boolean onlyInheritable) {
+        Map<Class<?>, CurrentInstance> map = instances.get();
+        if (map == null) {
+            return Collections.emptyMap();
+        } else {
+            Map<Class<?>, CurrentInstance> copy = new HashMap<Class<?>, CurrentInstance>();
+            for (Class<?> c : map.keySet()) {
+                CurrentInstance ci = map.get(c);
+                if (ci.inheritable || !onlyInheritable) {
+                    copy.put(c, ci);
+                }
+            }
+            return copy;
+        }
+    }
+
+    /**
      * Sets thread locals for the UI and all related classes
      * 
      * @param ui