Original felix/feat/dual based locking

[OriginalFelix]

Description

• Separated UI and Session Locking
• Fixed a Bug where whenever another UI-Class has been Configured the Configuration was ignored.

Fixes # (issue)
The issue was whenever a UI was freezing, all other UIs in the same VaadinSession were freezed as well.

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.
• I have performed self-review and corrected misspellings.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[github-actions]

Format Checker Report

There are 8 files with format errors

• To see a complete report of formatting issues, download the differences artifact

• To fix the build, please run mvn spotless:apply in your branch and commit the changes.

• Optionally you might add the following line in your .git/hooks/pre-commit file:

  mvn spotless:apply
  

Here is the list of files with format issues in your PR:

flow-server/src/main/java/com/vaadin/flow/component/UI.java
flow-server/src/main/java/com/vaadin/flow/server/AbstractFutureAccess.java
flow-server/src/main/java/com/vaadin/flow/server/AccessSupport.java
flow-server/src/main/java/com/vaadin/flow/server/FutureAccess.java
flow-server/src/main/java/com/vaadin/flow/server/UIFutureAccess.java
flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java
flow-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java

[Legioth]

Thanks for getting this initiative started!

I haven't looked at the code since there's one high-level design question that would be good to clarify first.

What has been done to avoid deadlocks if one thread has session A locked and does something that tries to lock UI B while another thread has UI B locked and does something that tries to lock session A?

Fixed a Bug where whenever another UI-Class has been Configured the Configuration was ignored.

That's not a bug. It's a conscious design decision. Furthermore, addressing multiple issues in the same PR is a bad practice since it can lead to a mess if one of the changes causes a regression because reverting the change will then also revert the unrelated changes.

[OriginalFelix]

Session locking does not interfere with the UI-Locking in this code anymore so there is no possibility of creating a deadlock that way. And if 2 UIs lock each other this should also not be a Problem....

[Legioth]

Closing expired UIs must be guarded by a session-wide lock since the list of open UIs is stored in the session. But the UI lock would also need to be acquired for closing the UI since it runs detach listeners for components in that UI. So how can that work without interaction between the session lock and the UI lock?

[Legioth]

I had a quick look at code and immediately noticed a red flag.

VaadinSession.unlock() holds the session lock while iterating through all UI instances and running ui.push() for all UIs that have automatic push mode enabled. The push() implementation does on the other hand have a this.checkHasLock(); check which might throw an exception in that case since I don't see that there would be anything that has acquired the lock for that UI.

It would be tempting to fix that by acquiring the UI lock inside the loop before calling push() but that would lead to additional problems:

1. Anything that needs a session lock, which is effectively all request handling that needs to find a UI instance, would be blocked while any UI in that session is locked.
2. Deadlock in case application code on another thread that currently holds the UI lock tries to acquire the session lock e.g. to read a session attribute.

So there are two possible scenarios here:

1. I'm missing something trivial
2. You're severely underestimating the complexity of this kind of change

I might be biased, but I see the 2nd option as much more likely.

[Legioth]

The core problem remains also after the latest changes. The implementation acquires UI locks while holding the session lock which negates most of the potential performance benefits since a single locked UI will still effectively block progress for all UIs in the session until that UI is unlocked. This is currently happening in VaadinSession::unlock but it will probably also be necessary in most other places where VaadinSession::getUIs or VaadinSession:getUIById are used.

In addition to the negated performance benefit, there's also still the same deadlock risk since application code may acquire the session lock while holding a UI lock (at least for accessing session attributes but there might also be other cases).

This just strengthens my impression that you are severely underestimating the complexity of this change. I'm sorry to say this but it seems like you are out of your league and the performance issues you see in your application are most likely a symptom of the same phenomenon and no amount of changes to the framework will help you get around that because it will not address the root cause.

I don't see any reason for us to further look at code-level issues in this PR unless you clearly show that you have a good understand of the big picture by presenting a plan for how to address the fundamental issue of locking UIs while holding the session lock.