fix: prevent deadlock when accessing FeatureFlags

[mcollovati]

Description

Synchronizing over FeatureFlags class in FeatureFlags.get could lead to a deadlock when the call comes from a supplier provided to VaadinContext.getAttribute() and concurrently from another request.

For example, in the linked issue:

• thread A invokes IndexHtmlRequestHandler.getCachedIndexHtmlDocument(), that locks the ServletContext through VaadinContext.getAttribute(...) providing a supplier that calls FeatureFlags.get()
• thread B concurrently executes IndexHtmlRequestHandler.getIndexHtmlDocument() that also calls FeatureFlags.get() that locks on FeaturFlags class

When B invokes FeatureFlags.get() -> VaadinContext.getAttribute() it has to wait because ServletContext is already locked A, but A is calling FeatureFlags.get() and it is waiting to lock on FeatureFlags class.

This change removes the synchronization on FeatureFlags class and relies on VaadinContext.getAttribute(Class, Supplier) internal synchronization to prevent FeatureFlag two be instanciated more than once.

It also adds test to ensure there are no regressions for #13962.

Fixes #17637

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]

Test Results

1 007 files  ±0  1 007 suites  ±0   1h 11m 30s ⏱️ + 13m 2s
6 417 tests +2  6 376 ✔️ +2  41 💤 ±0  0 ❌ ±0 
6 674 runs  +5  6 626 ✔️ +5  48 💤 ±0  0 ❌ ±0 

Results for commit 133a737. ± Comparison against base commit 7dc87e4.

♻️ This comment has been updated with latest results.

[tltv]

Are these new tests supposed to fail without this fix? When I run new tests without the fix, tests passed unexpectedly.

Also when I added the test from the issue #17637 in VaadinServletContextTest like suggested, it unexpectedly failed also with the fix. Not sure yet if this is some local problem, but its probably good to verify. How it behaves for you @mcollovati?

[mcollovati]

Without the fix, get_concurrentAccess_servletContextLock_noDeadlock always fails on my local because of the deadlock.

get_concurrentAccess_vaadinContextLock_noDeadlock passes because it is there only to prevent a regression, since FeatureFlags.get does not synchronize on VaadinContext anymore.

The test in the issue should be the same as get_concurrentAccess_servletContextLock_noDeadlock, so I don't know why it fails for you

[mcollovati]

Now I see. If you added the test to VaadinServletContextTest the problem is that the async tasks fail because Lookup is missing in the context.

[ForkJoinPool.commonPool-worker-2] ERROR com.vaadin.experimental.FeatureFlagsTest - Future failed
java.util.concurrent.CompletionException: java.lang.NullPointerException: Cannot invoke "com.vaadin.flow.di.Lookup.lookup(java.lang.Class)" because "this.lookup" is null
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315)
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1807)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1796)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)
Caused by: java.lang.NullPointerException: Cannot invoke "com.vaadin.flow.di.Lookup.lookup(java.lang.Class)" because "this.lookup" is null
	at com.vaadin.experimental.FeatureFlags.loadProperties(FeatureFlags.java:179)
	at com.vaadin.experimental.FeatureFlags.<init>(FeatureFlags.java:92)
	at com.vaadin.experimental.FeatureFlags.lambda$get$0(FeatureFlags.java:154)
	at com.vaadin.flow.server.VaadinServletContext.getAttribute(VaadinServletContext.java:73)
	at com.vaadin.experimental.FeatureFlags.get(FeatureFlags.java:152)
	at com.vaadin.flow.server.VaadinServletContextTest.lambda$getAttribute_supplierAccessFeatureFlag_deadlock$7(VaadinServletContextTest.java:176)
	at com.vaadin.flow.server.VaadinServletContext.getAttribute(VaadinServletContext.java:73)
	at com.vaadin.flow.server.VaadinServletContextTest.lambda$getAttribute_supplierAccessFeatureFlag_deadlock$8(VaadinServletContextTest.java:175)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804)
	... 6 more

You should add whenComplete() to the CompletableFuture to see the error

whenComplete( ((unused, throwable) -> {
            if (throwable != null) {
                throwable.printStackTrace();
            }
        })

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.2.0.beta1 and is also targeting the upcoming stable 24.2.0 version.