Skip to content

Commit

Permalink
fix: VaadinSessionScopes for all sessions are destroyed when any sing…
Browse files Browse the repository at this point in the history
…le session expires #20092 (#20103)

* 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: caalador <[email protected]>
Co-authored-by: Teppo Kurki <[email protected]>
  • Loading branch information
3 people authored Oct 9, 2024
1 parent e554b45 commit 2d42aad
Show file tree
Hide file tree
Showing 11 changed files with 105 additions and 122 deletions.
33 changes: 21 additions & 12 deletions flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -621,14 +622,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) {
Expand Down Expand Up @@ -684,18 +692,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);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -78,6 +81,8 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {

volatile boolean sessionClosedExplicitly = false;

final List<SessionDestroyListener> destroyListeners = new CopyOnWriteArrayList<>();

/**
* Configuration for the session.
*/
Expand Down Expand Up @@ -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})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";

/**
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}

/**
Expand All @@ -70,7 +61,7 @@ public void fireSessionDestroy() {
* the vaadin service session destroy listener
*/
public void addDestroyListener(SessionDestroyListener listener) {
destroyListeners.add(listener);
this.addSessionDestroyListener(listener);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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) {
Expand Down Expand Up @@ -146,9 +136,6 @@ private void destroy() {
routeStores.clear();
} finally {
session.unlock();
if (sessionDestroyListenerRegistration != null) {
sessionDestroyListenerRegistration.remove();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -96,9 +86,6 @@ private void destroy() {
uiStores.clear();
} finally {
session.unlock();
if (sessionDestroyListenerRegistration != null) {
sessionDestroyListenerRegistration.remove();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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());
Expand Down
Loading

0 comments on commit 2d42aad

Please sign in to comment.