VaadinSessionScopes for all sessions are destroyed when any single session expires #20092
Comments
|
Thanks for reporting the issue! Just to add some context: are you using the vaadin-spring add-on but with a custom |
I'm doing pretty much the same thing as in https://github.com/archiecobbs/vaadin-grid-load-fail-issue
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>vaadin-spring</artifactId>
</dependency>
...
<!-- Import vaadin-bom and vaadin-spring-bom which define all the Vaadin dependencies -->
<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>vaadin-bom</artifactId>
<version>${vaadin.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>vaadin-spring-bom</artifactId>
<version>${vaadin.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>Thanks. |
That's correct for the } else {
sessionDestroyListenerRegistration = session.getService()
.addSessionDestroyListener(event -> destroy());
}In the stack trace you can see that the In that bit of code, |
|
FYI, I verified that this patch fixes the bug for me: --- vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinSessionScope.java 2024-09-28 10:11:23.962928425 -0500
+++ vaadin-spring/src/main/java/com/vaadin/flow/spring/scopes/VaadinSessionScope.java 2024-09-28 10:46:20.279730421 -0500
@@ -50,7 +50,10 @@
.addDestroyListener(event -> destroy());
} else {
sessionDestroyListenerRegistration = session.getService()
- .addSessionDestroyListener(event -> destroy());
+ .addSessionDestroyListener(event -> {
+ if (event.getSession() == session)
+ destroy();
+ });
}
}
|
Yeah. I realized that immediately after posting so I deleted my post as it didn't really provide any value.
Good to have it verified but note that this isn't really a proper fix due to the performance characteristics of looping through all open sessions to check that new condition whenever one session is destroyed. Additionally, the listeners are kept in a If I remember correctly, this is the reason why the special implementation was originally added to |
Hang on a sec... you are conflating two distinct things: Bug 1: It's inefficient to use This discussion is about Bug 2. Of course Bug 1 is also a valid concern (and there should probably be a separate issue created for it) and if Bug 1 gets fixed that will surely fix Bug 2 as a side-effect, but as of now that has yet to happen so Bug 2 is still valid.
Regarding Bug 1, I have noticed this problem in the past several times in various guises. It's always the same pattern: there is some back-end resource that is allocated per-session, and we want to guarantee that it is properly cleaned up when the session disappears for any reason. But you can't just ask the How about this? We add a method like this to /**
* Adds a listener that gets notified when this session is destroyed.
* This session must be currently in state {@link VaadinSessionState#OPEN}.
* <p>
* This session will be locked and its {@link UI}s will have been removed
* when the listener is called.
*
* @param listener the session destroy listener
* @return a handle that can be used for removing the listener
* @throws IllegalStateException if this session is not currently open
* @see VaadinService#addSessionDestroyListener
*/
public Registration addDestroyListener(SessionDestroyListener listener);This could be implemented by adding a list of session-specific listeners to
Back to Bug 2: Other than the fact that it may someday be made obsolete by a fix to Bug 1, do you have any problems with my patch to fix Bug 2? |
|
The main problem with your suggested fix for Bug 2 is that it will lead to O(n^2) performance due to Bug 1. I would suggest that we just bite the bullet right away and add an API for listening to the death of a specific session instance and then use that to fix Bug 2. The implementation is relatively trivial and it already exists in The other thing on my mind is to declare it unsupported to use Vaadin's Spring scopes without also using Vaadin's Spring Servlet. That combination is apparently not widely used since it's taken this long for anyone to report the inevitable issue with disappearing bean stores. |
Great! I look forward to seeing that happen.
I understand and agree with your general negative reaction here, but I believe that you're assigning the blame in the wrong place. There is nothing in any of the Vaadin "scope" implementations that cares about the specifics of the servlet. Instead, the design problem here is evidenced by the presence of the In other words, I'm agreeing with your statement that this capability should be moved up from Would you like me to prototype a PR for this or would you like to do it? Thanks. |
Please go ahead if you want! 👍
I'm looking at this from a product management perspective. We can surely fix this one specific design issue but there's no telling what comes next unless we also invest into improving test coverage for this configuration. I'm not sure that would be a worthwhile investment since the configuration seems to be quite a special case that isn't widely used. |
…le 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 deprepcation. Fixes vaadin#20092.
Thanks - I created #20103.
My opinion only and with all due respect: The product management problem here is that Vaadin doesn't clearly define the boundary between Vaadin and Spring. As a result, every time I upgrade to a new version of Vaadin, everything Spring-related breaks because I'm not "doing Spring" exactly the way you do (which is apparently the only way you do). For example, Vaadin seems to assume Spring boot everywhere (see previous comments). Yet, your documentation doesn't actually declare that restriction. OK, so... what does that mean?? Moreover, the documentation is lacking with regards to how exactly Vaadin does integrate with Spring. Instead of defining how the various Vaadin components connect to Spring, and what particular functions they have, and how to enable or disable these functions, the documentation just says "copy this example Spring boot project and go from there" or "just add this starter and everything will work". For example, my application uses End of rant, my apologies. |
|
To paraphrase Tolstoy: All Spring Boot apps are alike; each Spring app without Spring Boot is broken in its own way. Focusing on Spring Boot helps us get 80% of the benefits for 20% of the effort. You are absolutely right that our guidance on how to set up a Spring app without Spring Boot is lacking. The thing I'm asking myself is that if we were to improve on that, then is there even a single such configuration that would let us cover 80% of the remaining cases with 20% of the effort? In other words, could we focus on a configuration that uses both scopes and |
|
It's a good question. I think the answer lies in doing the necessary legwork to decompose the problem space into orthogonal chunks. Spring Boot is designed so that users can apply the "opinionated defaults" provided by library authors for their libraries. It's not designed for library authors to mandate "opinionated defaults" for other libraries on behalf of the user - which is what Vaadin seems to be doing (and moreover without much explanation). You are a library provider, not a user, so you shouldn't be assuming anything about how the user wants to deploy their other libraries. The good news is that Spring is, overall, very well-designed and modular, and uses the "open/closed" design philosophy. For example, it's possible to implement a When Vaadin defines a
Another example is Vaadin's integration with Spring Security. I had a heck of a time trying to figure out how to add a REST API to a Vaadin/Spring application using In my ideal world, here is what I'd like to see wrt. Vaadin and Spring: A list of each of these "components" all properly modularized, a description of what they do, and a list dependencies between them. Then you could just say that "The Vaadin Spring Boot Starter automatically pulls in and configures these components A, B, and C like so... ". Any person used to dealing with Spring will be able to understand what that means. Right now, there seems to just be a bunch of undocumented magic, which is the opposite of the design philosophy of Spring and therefore frustrating to those familiar with using it. |
Description of the bug
After upgrading to Vaadin 24, I noticed a strange new warning:
The bean
MyBeanis a bean annotated with@Scope(VaadinSessionScope.VAADIN_SESSION_SCOPE_NAME), i.e., it is a session-scoped bean.I added some debugging and this is the stack trace when that error is emitted:
Basically what is happening is that session A is being shutdown, but this is causing the Spring scope associated with session B to be shutdown.
This appears to be the result of some incorrect logic in
VaadinSessionScope:The problem is that the method
VaadinService.addSessionDestroyListener()will deliver a notification for every session that is destroyed, butVaadinSessionScopeshould only be concerned with its own session. So it needs to ignore notifications for other, unrelated sessions, e.g., like this:As a result of this bug, if there are two
VaadinSessions and one of them expires, both of them will have theirVaadinSessionScope's destroyed. One particular side effect is that beans in the still-alive session aredestroy()'ed and in the process they may try to lock theirVaadinSession, but the deadVaadinSession's lock is already locked, generating the warning. Of course all kinds of other disasters could happen depending on the specific application.Expected behavior
A session's Spring scope should not be shutdown when some other session expires.
Minimal reproducible example
None currently available; the problem should be obvious.
Versions
The text was updated successfully, but these errors were encountered: