fix: prevent deadlock when accessing FeatureFlags (#17645)

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

Author: mcollovati

flow-dnd/src/test/java/com/vaadin/flow/component/dnd/AbstractDnDUnitTest.java

@@ -18,10 +18,12 @@
 
 import java.util.Collections;
 import java.util.Properties;
+import java.util.function.Supplier;
 
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.ArgumentMatchers;
 import org.mockito.Mockito;
 
 import com.vaadin.flow.component.Component;
@@ -52,6 +54,9 @@ public void setup() {
 
         Lookup lookup = Mockito.mock(Lookup.class);
         Mockito.when(context.getAttribute(Lookup.class)).thenReturn(lookup);
+        Mockito.when(context.getAttribute(ArgumentMatchers.any(Class.class),
+                ArgumentMatchers.any(Supplier.class)))
+                .then(i -> i.getArgument(1, Supplier.class).get());
 
         DefaultDeploymentConfiguration configuration = new DefaultDeploymentConfiguration(
                 appConfig, VaadinServlet.class, new Properties());

flow-server/src/main/java/com/vaadin/experimental/FeatureFlags.java

@@ -28,15 +28,15 @@
 import java.util.Optional;
 import java.util.Properties;
 
+import org.apache.commons.io.FileUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import com.vaadin.flow.di.Lookup;
 import com.vaadin.flow.di.ResourceProvider;
 import com.vaadin.flow.server.VaadinContext;
 import com.vaadin.flow.server.startup.ApplicationConfiguration;
 
-import org.apache.commons.io.FileUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 /**
  * Tracks available feature flags and their status.
  *
@@ -132,21 +132,15 @@ public FeatureFlags getFeatureFlags() {
     public static FeatureFlags get(final VaadinContext context) {
         assert context != null;
 
-        FeatureFlagsWrapper attribute;
-        synchronized (FeatureFlags.class) {
-            attribute = context.getAttribute(FeatureFlagsWrapper.class);
-
-            if (attribute == null) {
-                final FeatureFlags featureFlags = new FeatureFlags(
-                        context.getAttribute(Lookup.class));
-                featureFlags.configuration = ApplicationConfiguration
-                        .get(context);
-                featureFlags.loadProperties();
-                attribute = new FeatureFlagsWrapper(featureFlags);
-                context.setAttribute(attribute);
-            }
-        }
-
+        FeatureFlagsWrapper attribute = context
+                .getAttribute(FeatureFlagsWrapper.class, () -> {
+                    final FeatureFlags featureFlags = new FeatureFlags(
+                            context.getAttribute(Lookup.class));
+                    featureFlags.configuration = ApplicationConfiguration
+                            .get(context);
+                    featureFlags.loadProperties();
+                    return new FeatureFlagsWrapper(featureFlags);
+                });
         return attribute.getFeatureFlags();
     }
 

flow-server/src/test/java/com/vaadin/experimental/FeatureFlagsTest.java

@@ -15,19 +15,25 @@
  */
 package com.vaadin.experimental;
 
-import net.jcip.annotations.NotThreadSafe;
 import java.io.File;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.function.BiConsumer;
+import java.util.function.Supplier;
 
+import net.jcip.annotations.NotThreadSafe;
 import org.apache.commons.io.FileUtils;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.mockito.Mockito;
+import org.slf4j.LoggerFactory;
 
 import com.vaadin.flow.di.Lookup;
 import com.vaadin.flow.di.ResourceProvider;
@@ -36,6 +42,7 @@
 import com.vaadin.flow.server.VaadinContext;
 import com.vaadin.flow.server.VaadinService;
 import com.vaadin.flow.server.startup.ApplicationConfiguration;
+import com.vaadin.flow.server.startup.ApplicationRouteRegistry;
 
 import static com.vaadin.experimental.FeatureFlags.PROPERTIES_FILENAME;
 
@@ -308,6 +315,96 @@ public void noFeatureFlagFile_noSystemPropertyProvided_allFeatureDisabled()
         }
     }
 
+    // https://github.com/vaadin/flow/issues/17637
+    @Test
+    public void get_concurrentAccess_servletContextLock_noDeadlock()
+            throws Exception {
+        BiConsumer<Void, Throwable> errorLogger = (unused, throwable) -> {
+            if (throwable != null) {
+                LoggerFactory.getLogger(FeatureFlagsTest.class)
+                        .error("Future failed", throwable);
+            }
+        };
+        context = new MockVaadinContext() {
+            @Override
+            public <T> T getAttribute(Class<T> type) {
+                doSleep();
+                return super.getAttribute(type);
+            }
+
+            @Override
+            public <T> T getAttribute(Class<T> type,
+                    Supplier<T> defaultValueSupplier) {
+                doSleep();
+                return super.getAttribute(type, defaultValueSupplier);
+            }
+
+            private void doSleep() {
+                try {
+                    Thread.sleep(5);
+                } catch (InterruptedException e) {
+                    throw new RuntimeException(e);
+                }
+            }
+        };
+        CountDownLatch latch = new CountDownLatch(2);
+        CompletableFuture<Void> directTask = CompletableFuture.runAsync(() -> {
+            FeatureFlags.get(context);
+            latch.countDown();
+        }).whenComplete(errorLogger);
+        CompletableFuture<Void> supplierTask = CompletableFuture
+                .runAsync(() -> {
+                    context.getAttribute(FeatureFlags.class, () -> {
+                        FeatureFlags out = FeatureFlags.get(context);
+                        latch.countDown();
+                        return out;
+                    });
+                }).whenComplete(errorLogger);
+        CompletableFuture.allOf(directTask, supplierTask);
+        Assert.assertTrue("Futures not completed, potential deadlock",
+                latch.await(1, TimeUnit.SECONDS));
+    }
+
+    // https://github.com/vaadin/flow/issues/13962
+    @Test
+    public void get_concurrentAccess_vaadinContextLock_noDeadlock()
+            throws Exception {
+        BiConsumer<Void, Throwable> errorLogger = (unused, throwable) -> {
+            if (throwable != null) {
+                LoggerFactory.getLogger(FeatureFlagsTest.class)
+                        .error("Future failed", throwable);
+            }
+        };
+        context = new MockVaadinContext();
+        CountDownLatch latch = new CountDownLatch(2);
+        CompletableFuture<Void> supplierTask = CompletableFuture
+                .runAsync(() -> {
+                    // Simulation of ApplicationRouteRegistry.getInstance()
+                    // locking on VaadinContext
+                    synchronized (context) {
+                        ApplicationRouteRegistry attribute = context
+                                .getAttribute(ApplicationRouteRegistry.class);
+                        if (attribute == null) {
+                            attribute = Mockito
+                                    .mock(ApplicationRouteRegistry.class);
+                            context.setAttribute(attribute);
+                        }
+                    }
+                    context.getAttribute(FeatureFlags.class, () -> {
+                        FeatureFlags out = FeatureFlags.get(context);
+                        latch.countDown();
+                        return out;
+                    });
+                }).whenComplete(errorLogger);
+        CompletableFuture<Void> directTask = CompletableFuture.runAsync(() -> {
+            FeatureFlags.get(context);
+            latch.countDown();
+        }).whenComplete(errorLogger);
+        CompletableFuture.allOf(directTask, supplierTask);
+        Assert.assertTrue("Futures not completed, potential deadlock",
+                latch.await(1, TimeUnit.SECONDS));
+    }
+
     private boolean hasUsageStatsEntry(String name) {
         return UsageStatistics.getEntries()
                 .filter(entry -> entry.getName().equals(name)).findFirst()
@@ -333,4 +430,5 @@ private void mockResourcesLocation() {
     private void createFeatureFlagsFile(String data) throws IOException {
         FileUtils.write(featureFlagsFile, data, StandardCharsets.UTF_8);
     }
+
 }

vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringClassesSerializableTest.java

@@ -18,6 +18,7 @@
 import java.io.Serializable;
 import java.util.Collections;
 import java.util.Properties;
+import java.util.function.Supplier;
 import java.util.stream.Stream;
 
 import org.junit.Assert;
@@ -158,6 +159,9 @@ private TestBeanStore createStore() {
         Mockito.when(appConfig.getPropertyNames())
                 .thenReturn(Collections.emptyEnumeration());
         VaadinContext context = Mockito.mock(VaadinContext.class);
+        Mockito.when(
+                context.getAttribute(Mockito.any(Class.class), Mockito.any()))
+                .then(i -> i.getArgument(1, Supplier.class).get());
         Mockito.when(context.getAttribute(
                 Mockito.eq(ApplicationConfiguration.class), Mockito.any()))
                 .thenReturn(appConfig);

vaadin-spring/src/test/java/com/vaadin/flow/spring/scopes/AbstractScopeTest.java

@@ -18,10 +18,12 @@
 import java.util.Collections;
 import java.util.Properties;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Supplier;
 
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Test;
+import org.mockito.ArgumentMatchers;
 import org.mockito.Mockito;
 import org.springframework.beans.factory.ObjectFactory;
 import org.springframework.beans.factory.config.Scope;
@@ -153,6 +155,10 @@ protected VaadinSession mockSession() {
         VaadinContext context = Mockito.mock(VaadinContext.class);
         Lookup lookup = Mockito.mock(Lookup.class);
         Mockito.when(context.getAttribute(Lookup.class)).thenReturn(lookup);
+        Mockito.when(context.getAttribute(ArgumentMatchers.any(Class.class),
+                ArgumentMatchers.any(Supplier.class)))
+                .then(i -> i.getArgument(1, Supplier.class).get());
+
         Mockito.when(appConfig.getContext()).thenReturn(context);
         DefaultDeploymentConfiguration config = new DefaultDeploymentConfiguration(
                 appConfig, getClass(), initParameters);