Fix: #506 - VaadinSessionScopedContext was no longer active#507
Fix: #506 - VaadinSessionScopedContext was no longer active#507OriginalFelix wants to merge 15 commits into
Conversation
…n inside any other thread than the UI-Thread.
|
|
|
Felix Grebe seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
mcollovati
left a comment
mcollovati
left a comment
There was a problem hiding this comment.
I added a couple of comments. Overall looks good; I'm just a bit sad that we couldn't move the detection to the VaadinExtension class to execute it eagerly.
|
Maybe a stupid idea, but what about programmatically defining an observer for var serviceScopedContext = new VaadinServiceScopedContext(beanManager);
afterBeanDiscovery.<ServiceInitEvent>addObserverMethod()
.observedType(ServiceInitEvent.class)
.notifyWith(instance -> {
sessionContext.onServiceInit(instance.getEvent());
});This could be added to |
…nPolicy to VaadinExtension. And added Tests for Policies.
OriginalFelix
force-pushed
the
OriginalFelix-VaadinSessionScopedContext
branch
from
c663bea to
ee8590c
Compare
| return session != null && session.hasLock(); | ||
| final VaadinSession session = VaadinSession.getCurrent(); | ||
| if (VaadinSessionScopedContext.getActivationPolicy() == null && session != null) { | ||
| activationPolicy.compareAndSet(null, determineVaadinSessionScopeActivationPolicy(session.getService())); |
There was a problem hiding this comment.
Why use a static field to store the policy?
Also, why we should call determineVaadinSessionScopeActivationPolicy again? The VaadinService init event should always be fired before any VaadinSession gets created.
Can you elaborate on this?
There was a problem hiding this comment.
@mcollovati yes, the problem here is that the Event is only fired, when the CDI-Plugin is deployed to a Servlet Container, but not inside the Unit-Tests. Maybe i'm missing something there?
There was a problem hiding this comment.
OK, so it's mostly an issue in how the tests are constructed, right?
If so, let me check.
The other question is if we should assume a single VaadinService or multiple ones (even if I guess this is an extremely rare if not inexistent scenario)
There was a problem hiding this comment.
Yes.
Is it possible to have multiple VaadinServices in the same war?
There was a problem hiding this comment.
Potentially, yes, by having multiple VaadinServlets. Actually I don't remember if this configuration is supported or not.
Anyway, I'll propose you another approach that should handle also that case (sorry for being so picky 😜)
The idea is to follow the same pattern applied to get AppShellRegistry (the following code is not tested, I just put down the ide):
- Define a
VaadinSessionActivationPolicyHolderclass with aVaadinSessionScopeActivationPolicy.Policy get(VaadinService vaadinService)method that resemblesAppShellRegistry.getInstance(): getVaadinContextfrom service and store the policy in a private wrapper class
class VaadinSessionActivationPolicyHolder {
public static VaadinSessionScopeActivationPolicy.Policy get(
VaadinService vaadinService) {
VaadinContext context = vaadinService.getContext();
synchronized (context) { // NOSONAR
PolicyWrapper attribute = context.getAttribute(PolicyWrapper.class);
if (attribute == null) {
VaadinSessionScopeActivationPolicy.Policy policy = determinePolicy(
vaadinService);
attribute = new PolicyWrapper(policy);
context.setAttribute(attribute);
}
return attribute.policy;
}
}
private static VaadinSessionScopeActivationPolicy.Policy determinePolicy(
VaadinService vaadinService) {
// current logic
}
private record PolicyWrapper(VaadinSessionScopeActivationPolicy.Policy policy) {
}
}-
In
VaadinServiceScopedContextadd aServiceInitEventobserver and callVaadinSessionActivationPolicyHolder.get(...). This way we will have the policy stored for everyVaadinServicethat is created. -
In
VaadinSessionScopedContext.isActivesimply callVaadinSessionActivationPolicyHolder.get(session.getService())and use that value
How does this sound?
EDIT: we need to carefully verify the synchronized stuff
There was a problem hiding this comment.
Or we can mimic/reuse what's done in VaadinServiceScopedContext.getContextualStorage to get the key
There was a problem hiding this comment.
Looks good, thank you!
I guess we can remove some redundant null checks from VaadinSessionScopedContext if VaadinSessionActivationPolicyHolder already handles them. And most likely we can drop getActivationPolicy as well.
Also, determinePolicy is never called with a null parameter, so that's another check that can be removed.
I wonder if VaadinSessionActivationPolicyHolder could be handled observice ServiceInitEvent and ServiceDestroyEvent events. But that's not a blocker.
|
Got the following error on shutdown |
|
I guess this only needs the service name to be computed outside the destroy listener. EDIT: nope. That is only one point; |
|
I did a quick test making EDIT: |
|
The last one was actually tricky! the ServletName requires the Servlet to be Initialized... The solution was to check for the State of the VaadinService and Cache in name into a final String - So it's being stored once it's initialized.... |
Well done! I'll make a final review on Monday, but I think we are close to having this ready. |
4 participants
Description
In a Multi-Threaded Application 1 View with 10 Threads for Example, with threads containing the UI and VaadinSession, the Threads could access the CDI-Scopes before the changes breaking changes in the isActive method.
These changes were reverted and instead the findContextualStorage Method was adapted to dynamically take the lock, if not already running on a UI-Thread. This simplifies the adaptation for the customers, due to them not having to change every single line of code where they accessing the VaadinSessionScoped beans.
Fixes #506
Type of change
Checklist