Skip to content

refactor: extract @VaadinSessionScoped activation into managed bean#515

Open
mcollovati wants to merge 2 commits into
mainfrom
feat/pluggable-vaadinsessionscope-context-activation
Open

refactor: extract @VaadinSessionScoped activation into managed bean#515
mcollovati wants to merge 2 commits into
mainfrom
feat/pluggable-vaadinsessionscope-context-activation

Conversation

@mcollovati
Copy link
Copy Markdown
Contributor

Move the activation predicate and storage-access logic out of VaadinSessionScopedContext into a nested ContextualStorageManager managed bean. The context becomes a thin delegator. As a side effect, applications may replace the bean via CDI @Specializes to adjust behavior — though the manager is intentionally undocumented and overriding it is at the integrator's own risk.

Adds integration tests covering the strict default and the lenient @Specializes scenario from a background thread that does not hold the session lock.

References #506

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 26, 2026

Test Results

131 tests  +9   131 ✅ +9   3m 39s ⏱️ +9s
 35 suites +2     0 💤 ±0 
 35 files   +2     0 ❌ ±0 

Results for commit 634cdd4. ± Comparison against base commit 96421da.

♻️ This comment has been updated with latest results.

@mcollovati mcollovati marked this pull request as ready for review May 27, 2026 06:05
@mshabarov mshabarov requested a review from heruan June 1, 2026 11:52
@mshabarov
Copy link
Copy Markdown
Contributor

Let's also follow the internal discussion in Slack.

Move the activation predicate and storage-access logic out of
`VaadinSessionScopedContext` into a nested `ContextualStorageManager`
managed bean. The context becomes a thin delegator. As a side
effect, applications may replace the bean via CDI `@Specializes` to
adjust behavior — though the manager is intentionally undocumented
and overriding it is at the integrator's own risk.

Adds integration tests covering the strict default and the lenient
`@Specializes` scenario from a background thread that does not hold
the session lock.

References #506
@mcollovati mcollovati force-pushed the feat/pluggable-vaadinsessionscope-context-activation branch from b55d903 to f3b3b0f Compare June 1, 2026 12:02
}, "background-event-fire");
thread.start();
try {
thread.join(5000);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this hold the lock for the whole 5 secs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dirty hack to avoid deadlocks in the test. Let me rewrite it in a better way

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored view and test

`SessionContextSpecializesView` joined the spawned background thread
with a 5 s timeout: in the lenient `@Specializes` deployment the
thread waits in `VaadinSession#accessSynchronously` for the session
lock held by the joining request thread, so an unbounded join would
deadlock. Every click stalled the full 5 s and the assertions raced
the still-running thread.

The click listener now only starts the thread, removing the lock
inversion entirely. The thread increments a completion counter in
its `finally` block, and the tests synchronize on it via a new
`waitForCount()` helper before asserting.
@mcollovati mcollovati requested a review from heruan June 3, 2026 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🔎Iteration reviews

Development

Successfully merging this pull request may close these issues.

3 participants