Fix flow issue #6349

[archiecobbs]

Description

Commands enqueued by VaadinSession.access() in general have nothing to do with each other. The only thing they have in common is they share the same VaadinSession (and, by implication, VaadinService).

Therefore, if command №1 invoked UI.setCurrent() and command №2 invokes UI.getCurrent(), command №2 should read null, not the random UI from command №1 that it has nothing to do with.

Fixes #6349

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change
  • Except JarContentsManagerTest.copyFilesFromJar_doNotUpdateFileIfContentIsTheSame which has nothing to do with this change.
• I have performed self-review and corrected misspellings.

[tepi]

Just to be sure: Is Flow erroneously setting random UI instances to the CurrentInstance value of UI, or is this happening only when the developer is calling UI.setCurrent() themselves within a session access command?

[archiecobbs]

Just to be sure: Is Flow erroneously setting random UI instances to the CurrentInstance value of UI, or is this happening only when the developer is calling UI.setCurrent() themselves within a session access command?

HI @tepi ,

The overall problem is that whatever UI (or VaadinRequest or VaadinResponse) happens to be leftover from a previous operation is not removed before the next task in the session's pending access queue is executed.

Note that a "previous operation" could be any of these:

• The handling of a normal request/response cycle
• A previous task in the queue (if the affected task is not the first task in the queue)
• Any other operation which requires a session lock/unlock cycle

As a result, when an enqueued session task invokes UI.getCurrent(), VaadinRequest.getCurrent(), or VaadinResponse.getCurrent(), it can get a "random" value when instead null should have been returned.

So it's not required that the first session task include an explicit call to UI.setCurrent(), it's only required that UI.setCurrent() is called as a side-effect of the previous operation (for example, handling a normal incoming request).

Here is an example of a sequence of events demonstrating one way this could happen:

• Vaadin HTTP request comes in and is handled by Thread A
• Thread A locks the session and sets the current UI, VaadinRequest, and VaadinResponse
• Background thread B wakes up and invokes VaadinSession.access(task)
• Because the session is already locked by thread A, task is added to the session's pending access queue
• Thread A finishes handling the request and unlocks the session
• On session unlock, the pending access queue is processed
• The task executes in thread A, but:
  • The previous UI, VaadinRequest, and VaadinResponse are still in context
  • Yet those three objects have nothing to do with task

[sonarcloud]

Quality Gate passed

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[mshabarov]

The change looks good to me and safe. Excellent that you added the test, it indeed fails when this patch isn't in place.

Thanks for your contribution 👍

[github-actions]

Test Results

1 142 files  ± 0  1 142 suites  ±0   1h 30m 37s ⏱️ + 6m 45s
7 465 tests + 1  7 415 ✅ + 1  50 💤 ±0  0 ❌ ±0 
7 831 runs  +51  7 771 ✅ +51  60 💤 ±0  0 ❌ ±0 

Results for commit 1df59e9. ± Comparison against base commit 1937249.