fix: VaadinSessionScopes for all sessions are destroyed when any sing…

…le session expires #20092 (#20103) (#20204) * fix: VaadinSessionScopes for all sessions are destroyed when any single session expires. * Add new method VaadinSession.addSessionDestroyListener() for when you just want one destroy notificatios for a single session, instead of geting notifications for all sessions via VaadinService.addSessionDestroyListener(). * Refactor the Spring scopes to take advantage of the new method to fix an inefficiency. * Mark SpringVaadinSession, which is now empty and useless, for deprecation. Fixes #20092. * Apply formatter. * Restore public method SpringVaadinSession.addDestroyListener(). * Restore public method SpringVaadinSession.fireSessionDestroy(). --------- Co-authored-by: Archie L. Cobbs <[email protected]> Co-authored-by: caalador <[email protected]> Co-authored-by: Teppo Kurki <[email protected]>
vaadin · Oct 9, 2024 · 13fb546 · 13fb546
1 parent c276c5c
commit 13fb546
Show file tree

Hide file tree

Showing 11 changed files with 105 additions and 122 deletions.
diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
@@ -46,6 +46,7 @@
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
 import org.slf4j.Logger;
@@ -589,14 +590,21 @@ public Registration addUIInitListener(UIInitListener listener) {
     /**
      * Adds a listener that gets notified when a Vaadin service session that has
      * been initialized for this service is destroyed.
+     *
      * <p>
      * The session being destroyed is locked and its UIs have been removed when
      * the listeners are called.
      *
+     * <p>
+     * This method delivers notifications for all associated sessions. To be
+     * notified for only one specific session, use
+     * {@link VaadinSession#addSessionDestroyListener}.
+     *
      * @param listener
      *            the vaadin service session destroy listener
      * @return a handle that can be used for removing the listener
      * @see #addSessionInitListener(SessionInitListener)
+     * @see VaadinSession#addSessionDestroyListener
      */
     public Registration addSessionDestroyListener(
             SessionDestroyListener listener) {
@@ -652,18 +660,19 @@ public void fireSessionDestroy(VaadinSession vaadinSession) {
             }
             SessionDestroyEvent event = new SessionDestroyEvent(
                     VaadinService.this, session);
-            for (SessionDestroyListener listener : sessionDestroyListeners) {
-                try {
-                    listener.sessionDestroy(event);
-                } catch (Exception e) {
-                    /*
-                     * for now, use the session error handler; in the future,
-                     * could have an API for using some other handler for
-                     * session init and destroy listeners
-                     */
-                    session.getErrorHandler().error(new ErrorEvent(e));
-                }
-            }
+            Stream.concat(session.destroyListeners.stream(),
+                    sessionDestroyListeners.stream()).forEach(listener -> {
+                        try {
+                            listener.sessionDestroy(event);
+                        } catch (Exception e) {
+                            /*
+                             * for now, use the session error handler; in the
+                             * future, could have an API for using some other
+                             * handler for session init and destroy listeners
+                             */
+                            session.getErrorHandler().error(new ErrorEvent(e));
+                        }
+                    });
 
             session.setState(VaadinSessionState.CLOSED);
         });

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java
@@ -25,13 +25,15 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedList;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Queue;
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.Future;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -47,6 +49,7 @@
 import com.vaadin.flow.internal.CurrentInstance;
 import com.vaadin.flow.internal.StateNode;
 import com.vaadin.flow.server.startup.ApplicationConfiguration;
+import com.vaadin.flow.shared.Registration;
 import com.vaadin.flow.shared.communication.PushMode;
 
 import jakarta.servlet.http.HttpSession;
@@ -78,6 +81,8 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
 
     volatile boolean sessionClosedExplicitly = false;
 
+    final List<SessionDestroyListener> destroyListeners = new CopyOnWriteArrayList<>();
+
     /**
      * Configuration for the session.
      */
@@ -489,6 +494,31 @@ public Collection<RequestHandler> getRequestHandlers() {
         return Collections.unmodifiableCollection(requestHandlers);
     }
 
+    /**
+     * Adds a listener that gets notified when this session is destroyed.
+     *
+     * <p>
+     * This session will be locked and its {@link UI}s will have been removed
+     * when the listener is called.
+     *
+     * <p>
+     * If this session is already closed, no notification is delivered.
+     *
+     * <p>
+     * This method only delivers notifications for this session. To also be
+     * notified about other sessions, use
+     * {@link VaadinService#addSessionDestroyListener}.
+     *
+     * @param listener
+     *            the session destroy listener
+     * @return a handle that can be used for removing the listener
+     * @see VaadinService#addSessionDestroyListener
+     */
+    public Registration addSessionDestroyListener(
+            SessionDestroyListener listener) {
+        return Registration.addAndRemove(destroyListeners, listener);
+    }
+
     /**
      * Gets the currently used session. The current session is automatically
      * defined when processing requests to the server (see {@link ThreadLocal})

diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringVaadinServletService.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringVaadinServletService.java
@@ -46,8 +46,6 @@ public class SpringVaadinServletService extends VaadinServletService {
 
     private final transient ApplicationContext context;
 
-    private final Registration serviceDestroyRegistration;
-
     static final String SPRING_BOOT_WEBPROPERTIES_CLASS = "org.springframework.boot.autoconfigure.web.WebProperties";
 
     /**
@@ -66,11 +64,6 @@ public SpringVaadinServletService(VaadinServlet servlet,
             ApplicationContext context) {
         super(servlet, deploymentConfiguration);
         this.context = context;
-        SessionDestroyListener listener = event -> sessionDestroyed(
-                event.getSession());
-        Registration registration = addSessionDestroyListener(listener);
-        serviceDestroyRegistration = addServiceDestroyListener(
-                event -> serviceDestroyed(registration));
     }
 
     @Override
@@ -105,21 +98,13 @@ public void init() throws ServiceException {
         uiInitListeners.values().forEach(this::addUIInitListener);
     }
 
+    // This method should be removed when the deprecated class
+    // SpringVaadinSession is removed
     @Override
     protected VaadinSession createVaadinSession(VaadinRequest request) {
         return new SpringVaadinSession(this);
     }
 
-    private void sessionDestroyed(VaadinSession session) {
-        assert session instanceof SpringVaadinSession;
-        ((SpringVaadinSession) session).fireSessionDestroy();
-    }
-
-    private void serviceDestroyed(Registration registration) {
-        registration.remove();
-        serviceDestroyRegistration.remove();
-    }
-
     @Override
     public URL getStaticResource(String path) {
         URL resource = super.getStaticResource(path);

diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringVaadinSession.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringVaadinSession.java
@@ -15,27 +15,21 @@
  */
 package com.vaadin.flow.spring;
 
-import java.util.List;
-import java.util.concurrent.CopyOnWriteArrayList;
-
-import com.vaadin.flow.server.SessionDestroyEvent;
 import com.vaadin.flow.server.SessionDestroyListener;
-import com.vaadin.flow.server.SessionInitListener;
 import com.vaadin.flow.server.VaadinService;
 import com.vaadin.flow.server.VaadinSession;
 
 /**
  * Vaadin session implementation for Spring.
  *
  * @author Vaadin Ltd
- *
+ * @deprecated No replacement planned
  */
+@Deprecated(forRemoval = true)
 public class SpringVaadinSession extends VaadinSession {
 
-    private final List<SessionDestroyListener> destroyListeners = new CopyOnWriteArrayList<>();
-
     /**
-     * Creates a new VaadinSession tied to a VaadinService.
+     * Creates a new SpringVaadinSession tied to a VaadinService.
      *
      * @param service
      *            the Vaadin service for the new session
@@ -48,10 +42,7 @@ public SpringVaadinSession(VaadinService service) {
      * Handles destruction of the session.
      */
     public void fireSessionDestroy() {
-        SessionDestroyEvent event = new SessionDestroyEvent(getService(), this);
-        destroyListeners.stream()
-                .forEach(listener -> listener.sessionDestroy(event));
-        destroyListeners.clear();
+        getService().fireSessionDestroy(this);
     }
 
     /**
@@ -70,7 +61,7 @@ public void fireSessionDestroy() {
      *            the vaadin service session destroy listener
      */
     public void addDestroyListener(SessionDestroyListener listener) {
-        destroyListeners.add(listener);
+        this.addSessionDestroyListener(listener);
     }
 
 }
diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinRouteScope.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinRouteScope.java
@@ -50,7 +50,6 @@
 import com.vaadin.flow.server.VaadinServletContext;
 import com.vaadin.flow.server.VaadinSession;
 import com.vaadin.flow.shared.Registration;
-import com.vaadin.flow.spring.SpringVaadinSession;
 import com.vaadin.flow.spring.annotation.RouteScopeOwner;
 
 /**
@@ -72,22 +71,13 @@ private static class RouteStoreWrapper implements Serializable {
 
         private final VaadinSession session;
 
-        private final Registration sessionDestroyListenerRegistration;
-
         private final Map<String, RouteBeanStore> routeStores;
 
         private RouteStoreWrapper(VaadinSession session) {
             assert session.hasLock();
             this.session = session;
+            session.addSessionDestroyListener(event -> destroy());
             routeStores = new HashMap<>();
-            if (session instanceof SpringVaadinSession) {
-                sessionDestroyListenerRegistration = null;
-                ((SpringVaadinSession) session)
-                        .addDestroyListener(event -> destroy());
-            } else {
-                sessionDestroyListenerRegistration = session.getService()
-                        .addSessionDestroyListener(event -> destroy());
-            }
         }
 
         private RouteBeanStore getBeanStore(UI ui) {
@@ -146,9 +136,6 @@ private void destroy() {
                 routeStores.clear();
             } finally {
                 session.unlock();
-                if (sessionDestroyListenerRegistration != null) {
-                    sessionDestroyListenerRegistration.remove();
-                }
             }
         }
 

diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinSessionScope.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinSessionScope.java
@@ -20,7 +20,6 @@
 
 import com.vaadin.flow.server.VaadinSession;
 import com.vaadin.flow.shared.Registration;
-import com.vaadin.flow.spring.SpringVaadinSession;
 
 /**
  * Implementation of Spring's
@@ -40,31 +39,15 @@ public class VaadinSessionScope extends AbstractScope {
 
     private static class SessionBeanStore extends BeanStore {
 
-        private final Registration sessionDestroyListenerRegistration;
-
         private SessionBeanStore(VaadinSession session) {
             super(session);
-            if (session instanceof SpringVaadinSession) {
-                sessionDestroyListenerRegistration = null;
-                ((SpringVaadinSession) session)
-                        .addDestroyListener(event -> destroy());
-            } else {
-                sessionDestroyListenerRegistration = session.getService()
-                        .addSessionDestroyListener(event -> destroy());
-            }
+            session.addSessionDestroyListener(event -> destroy());
         }
 
         @Override
         Void doDestroy() {
-            try {
-                getVaadinSession().setAttribute(BeanStore.class, null);
-                super.doDestroy();
-            } finally {
-                if (sessionDestroyListenerRegistration != null) {
-                    sessionDestroyListenerRegistration.remove();
-                }
-            }
-            return null;
+            getVaadinSession().setAttribute(BeanStore.class, null);
+            return super.doDestroy();
         }
     }
 

diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinUIScope.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinUIScope.java
@@ -26,7 +26,6 @@
 import com.vaadin.flow.component.UI;
 import com.vaadin.flow.server.VaadinSession;
 import com.vaadin.flow.shared.Registration;
-import com.vaadin.flow.spring.SpringVaadinSession;
 
 /**
  * Implementation of Spring's
@@ -48,22 +47,13 @@ private static class UIStoreWrapper
 
         private final VaadinSession session;
 
-        private final Registration sessionDestroyListenerRegistration;
-
         private final Map<Integer, BeanStore> uiStores;
 
         private UIStoreWrapper(VaadinSession session) {
             assert session.hasLock();
             uiStores = new HashMap<>();
             this.session = session;
-            if (session instanceof SpringVaadinSession) {
-                sessionDestroyListenerRegistration = null;
-                ((SpringVaadinSession) session)
-                        .addDestroyListener(event -> destroy());
-            } else {
-                sessionDestroyListenerRegistration = session.getService()
-                        .addSessionDestroyListener(event -> destroy());
-            }
+            session.addSessionDestroyListener(event -> destroy());
         }
 
         @Override
@@ -96,9 +86,6 @@ private void destroy() {
                 uiStores.clear();
             } finally {
                 session.unlock();
-                if (sessionDestroyListenerRegistration != null) {
-                    sessionDestroyListenerRegistration.remove();
-                }
             }
         }
 

diff --git a/vaadin-spring/src/test/java/com/vaadin/flow/spring/scopes/AbstractScopeTest.java b/vaadin-spring/src/test/java/com/vaadin/flow/spring/scopes/AbstractScopeTest.java
@@ -18,6 +18,7 @@
 import java.util.Collections;
 import java.util.Properties;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Supplier;
 
 import org.junit.After;
@@ -35,7 +36,6 @@
 import com.vaadin.flow.server.VaadinSession;
 import com.vaadin.flow.server.VaadinSessionState;
 import com.vaadin.flow.server.startup.ApplicationConfiguration;
-import com.vaadin.flow.spring.SpringVaadinSession;
 
 import static org.mockito.Mockito.doCallRealMethod;
 import static org.mockito.Mockito.times;
@@ -46,12 +46,18 @@ public abstract class AbstractScopeTest {
 
     private VaadinSession session;
 
-    public static class TestSession extends SpringVaadinSession {
+    public static class TestSession extends VaadinSession {
+
+        private final ReentrantLock lock = new ReentrantLock();
 
         public TestSession() {
-            super(Mockito.mock(VaadinService.class));
+            super(Mockito.spy(VaadinService.class));
         }
 
+        @Override
+        public ReentrantLock getLockInstance() {
+            return this.lock;
+        }
     }
 
     @After
@@ -131,7 +137,7 @@ protected void registerDestructionCallback_currentScopeIsSet_objectIsStored(
 
     @SuppressWarnings("unchecked")
     protected VaadinSession mockSession() {
-        SpringVaadinSession session = Mockito.mock(TestSession.class,
+        VaadinSession session = Mockito.mock(TestSession.class,
                 Mockito.withSettings().useConstructor());
         doCallRealMethod().when(session).setAttribute(Mockito.any(Class.class),
                 Mockito.any());