Fix deadlock issue. (#2235)

vkryachko · web-flow · commit f2019076fe12 · 2020-12-07T21:23:00.000-05:00
* Fix deadlock issue.

The deadlock was arising when components are initialized concurrently
from different threads all waiting for `Lazy.get` to resolve which would
never succeed.

* Increase timeout, add licenses.

* Added comments.
diff --git a/firebase-components/src/main/java/com/google/firebase/components/ComponentRuntime.java b/firebase-components/src/main/java/com/google/firebase/components/ComponentRuntime.java
@@ -34,6 +34,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * Main entry point to the component system.
@@ -48,7 +49,7 @@ public class ComponentRuntime extends AbstractComponentContainer implements Comp
   private final Map<Class<?>, LazySet<?>> lazySetMap = new HashMap<>();
   private final List<Provider<ComponentRegistrar>> unprocessedRegistrarProviders;
   private final EventBus eventBus;
-  private Boolean eagerComponentsInitializedWith = null;
+  private final AtomicReference<Boolean> eagerComponentsInitializedWith = new AtomicReference<>();
 
   /**
    * Creates an instance of {@link ComponentRuntime} for the provided {@link ComponentRegistrar}s
@@ -90,49 +91,63 @@ private ComponentRuntime(
     discoverComponents(componentsToAdd);
   }
 
-  private synchronized void discoverComponents(List<Component<?>> componentsToAdd) {
-
-    Iterator<Provider<ComponentRegistrar>> iterator = unprocessedRegistrarProviders.iterator();
-    while (iterator.hasNext()) {
-      Provider<ComponentRegistrar> provider = iterator.next();
-      try {
-        ComponentRegistrar registrar = provider.get();
-        if (registrar != null) {
-          componentsToAdd.addAll(registrar.getComponents());
+  private void discoverComponents(List<Component<?>> componentsToAdd) {
+    // During discovery many things need to happen, of which "Deferred resolution" and "Set binding
+    // updates" are of most interest. During these phases we execute "client" code(i.e. the code of
+    // SDKs participating in Components DI), this code can indirectly end up calling back into the
+    // ComponentRuntime which, without proper care, can result in a deadlock. For this reason,
+    // instead of executing such code in the synchronized block below, we store it in a list and
+    // execute right after the synchronized section.
+    List<Runnable> runAfterDiscovery = new ArrayList<>();
+    synchronized (this) {
+      Iterator<Provider<ComponentRegistrar>> iterator = unprocessedRegistrarProviders.iterator();
+      while (iterator.hasNext()) {
+        Provider<ComponentRegistrar> provider = iterator.next();
+        try {
+          ComponentRegistrar registrar = provider.get();
+          if (registrar != null) {
+            componentsToAdd.addAll(registrar.getComponents());
+            iterator.remove();
+          }
+        } catch (InvalidRegistrarException ex) {
           iterator.remove();
+          Log.w(ComponentDiscovery.TAG, "Invalid component registrar.", ex);
         }
-      } catch (InvalidRegistrarException ex) {
-        iterator.remove();
-        Log.w(ComponentDiscovery.TAG, "Invalid component registrar.", ex);
       }
-    }
 
-    if (components.isEmpty()) {
-      CycleDetector.detect(componentsToAdd);
-    } else {
-      ArrayList<Component<?>> allComponents = new ArrayList<>(this.components.keySet());
-      allComponents.addAll(componentsToAdd);
-      CycleDetector.detect(allComponents);
-    }
+      if (components.isEmpty()) {
+        CycleDetector.detect(componentsToAdd);
+      } else {
+        ArrayList<Component<?>> allComponents = new ArrayList<>(this.components.keySet());
+        allComponents.addAll(componentsToAdd);
+        CycleDetector.detect(allComponents);
+      }
 
-    for (Component<?> component : componentsToAdd) {
-      Lazy<?> lazy =
-          new Lazy<>(
-              () ->
-                  component.getFactory().create(new RestrictedComponentContainer(component, this)));
+      for (Component<?> component : componentsToAdd) {
+        Lazy<?> lazy =
+            new Lazy<>(
+                () ->
+                    component
+                        .getFactory()
+                        .create(new RestrictedComponentContainer(component, this)));
 
-      components.put(component, lazy);
-    }
+        components.put(component, lazy);
+      }
 
-    processInstanceComponents(componentsToAdd);
-    processSetComponents();
-    processDependencies();
+      runAfterDiscovery.addAll(processInstanceComponents(componentsToAdd));
+      runAfterDiscovery.addAll(processSetComponents());
+      processDependencies();
+    }
+    for (Runnable runnable : runAfterDiscovery) {
+      runnable.run();
+    }
     maybeInitializeEagerComponents();
   }
 
   private void maybeInitializeEagerComponents() {
-    if (eagerComponentsInitializedWith != null) {
-      doInitializeEagerComponents(eagerComponentsInitializedWith);
+    Boolean isDefaultApp = eagerComponentsInitializedWith.get();
+    if (isDefaultApp != null) {
+      doInitializeEagerComponents(components, isDefaultApp);
     }
   }
 
@@ -153,7 +168,8 @@ private static <T> List<T> iterableToList(Iterable<T> iterable) {
     return result;
   }
 
-  private void processInstanceComponents(List<Component<?>> componentsToProcess) {
+  private List<Runnable> processInstanceComponents(List<Component<?>> componentsToProcess) {
+    ArrayList<Runnable> runnables = new ArrayList<>();
     for (Component<?> component : componentsToProcess) {
       if (!component.isValue()) {
         continue;
@@ -169,14 +185,16 @@ private void processInstanceComponents(List<Component<?>> componentsToProcess) {
           OptionalProvider<Object> deferred = (OptionalProvider<Object>) existingProvider;
           @SuppressWarnings("unchecked")
           Provider<Object> castedProvider = (Provider<Object>) provider;
-          deferred.set(castedProvider);
+          runnables.add(() -> deferred.set(castedProvider));
         }
       }
     }
+    return runnables;
   }
 
   /** Populates lazySetMap to make set components available for consumption via set dependencies. */
-  private void processSetComponents() {
+  private List<Runnable> processSetComponents() {
+    ArrayList<Runnable> runnables = new ArrayList<>();
     Map<Class<?>, Set<Provider<?>>> setIndex = new HashMap<>();
     for (Map.Entry<Component<?>, Provider<?>> entry : components.entrySet()) {
       Component<?> component = entry.getKey();
@@ -202,10 +220,11 @@ private void processSetComponents() {
       } else {
         LazySet<Object> existingSet = (LazySet<Object>) lazySetMap.get(entry.getKey());
         for (Provider<?> provider : entry.getValue()) {
-          existingSet.add((Provider<Object>) provider);
+          runnables.add(() -> existingSet.add((Provider<Object>) provider));
         }
       }
     }
+    return runnables;
   }
 
   @Override
@@ -243,17 +262,28 @@ public synchronized <T> Provider<Set<T>> setOfProvider(Class<T> anInterface) {
    * <p>Should be called at an appropriate time in the owner's lifecycle.
    *
    * <p>Note: the method is idempotent.
+   *
+   * <p>Warning: it's important that this method is not synchronized as it could cause a deadlock if
+   * another SDK is initializing in another thread.
    */
-  public synchronized void initializeEagerComponents(boolean isDefaultApp) {
-    if (eagerComponentsInitializedWith != null) {
+  public void initializeEagerComponents(boolean isDefaultApp) {
+    if (!eagerComponentsInitializedWith.compareAndSet(null, isDefaultApp)) {
       return;
     }
-    eagerComponentsInitializedWith = isDefaultApp;
-    doInitializeEagerComponents(isDefaultApp);
+
+    // we copy the map under a lock to avoid a race condition.
+    // Note that we cannot use a ConcurrentHashMap as it is broken on older Android versions, see:
+    // https://issuetracker.google.com/issues/37042460
+    HashMap<Component<?>, Provider<?>> componentsCopy;
+    synchronized (this) {
+      componentsCopy = new HashMap<>(components);
+    }
+    doInitializeEagerComponents(componentsCopy, isDefaultApp);
   }
 
-  private void doInitializeEagerComponents(boolean isDefaultApp) {
-    for (Map.Entry<Component<?>, Provider<?>> entry : components.entrySet()) {
+  private void doInitializeEagerComponents(
+      Map<Component<?>, Provider<?>> componentsToInitialize, boolean isDefaultApp) {
+    for (Map.Entry<Component<?>, Provider<?>> entry : componentsToInitialize.entrySet()) {
       Component<?> component = entry.getKey();
       Provider<?> provider = entry.getValue();
 
@@ -266,9 +296,11 @@ private void doInitializeEagerComponents(boolean isDefaultApp) {
   }
 
   @Override
-  public synchronized void discoverComponents() {
-    if (unprocessedRegistrarProviders.isEmpty()) {
-      return;
+  public void discoverComponents() {
+    synchronized (this) {
+      if (unprocessedRegistrarProviders.isEmpty()) {
+        return;
+      }
     }
     discoverComponents(new ArrayList<>());
   }
diff --git a/firebase-components/src/test/java/com/google/firebase/components/threading/AnotherEagerComponent.java b/firebase-components/src/test/java/com/google/firebase/components/threading/AnotherEagerComponent.java
@@ -0,0 +1,17 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.components.threading;
+
+public class AnotherEagerComponent {}
diff --git a/firebase-components/src/test/java/com/google/firebase/components/threading/DeadlockTests.java b/firebase-components/src/test/java/com/google/firebase/components/threading/DeadlockTests.java
@@ -0,0 +1,131 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.components.threading;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import com.google.firebase.components.Component;
+import com.google.firebase.components.ComponentRegistrar;
+import com.google.firebase.components.ComponentRuntime;
+import com.google.firebase.components.Dependency;
+import com.google.firebase.inject.Provider;
+import java.util.Collections;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class DeadlockTests {
+  @Test(timeout = 2000)
+  public void concurrentInitialization_withValueComponents_shouldNotDeadlock() {
+    ComponentRuntime runtime =
+        ComponentRuntime.builder(Runnable::run)
+            .addComponent(
+                Component.builder(EagerComponent.class)
+                    .add(Dependency.requiredProvider(AnotherEagerComponent.class))
+                    .factory(
+                        c -> {
+                          Provider<AnotherEagerComponent> another =
+                              c.getProvider(AnotherEagerComponent.class);
+                          CountDownLatch latch = new CountDownLatch(1);
+                          Executors.newSingleThreadExecutor()
+                              .execute(
+                                  () -> {
+                                    another.get();
+                                    latch.countDown();
+                                  });
+                          try {
+                            latch.await();
+                          } catch (InterruptedException e) {
+                            Thread.currentThread().interrupt();
+                            throw new AssertionError("Deadlock detected.", e);
+                          }
+                          return new EagerComponent();
+                        })
+                    .alwaysEager()
+                    .build())
+            .addComponent(
+                Component.builder(AnotherEagerComponent.class)
+                    .add(Dependency.optionalProvider(String.class))
+                    .factory(
+                        c -> {
+                          c.getProvider(String.class);
+                          return new AnotherEagerComponent();
+                        })
+                    .alwaysEager()
+                    .build())
+            .build();
+    runtime.initializeEagerComponents(true);
+  }
+
+  @Test(timeout = 2000)
+  public void concurrentInitialization_withDeferredComponents_shouldNotDeadlock()
+      throws InterruptedException {
+    CountDownLatch stringDeferredInjected = new CountDownLatch(1);
+    @SuppressWarnings("unchecked")
+    Provider<ComponentRegistrar> missingRegistrar = mock(Provider.class);
+    ComponentRuntime runtime =
+        ComponentRuntime.builder(Runnable::run)
+            .addComponent(
+                Component.builder(EagerComponent.class)
+                    .add(Dependency.deferred(String.class))
+                    .factory(
+                        c -> {
+                          c.getDeferred(String.class)
+                              .whenAvailable(
+                                  p -> {
+                                    p.get();
+                                    stringDeferredInjected.countDown();
+                                  });
+                          return new EagerComponent();
+                        })
+                    .alwaysEager()
+                    .build())
+            .addLazyComponentRegistrars(Collections.singleton(missingRegistrar))
+            .build();
+    runtime.initializeEagerComponents(true);
+
+    when(missingRegistrar.get())
+        .thenReturn(
+            () ->
+                Collections.singletonList(
+                    Component.builder(String.class)
+                        .add(Dependency.optionalProvider(Double.class))
+                        .factory(
+                            c -> {
+                              CountDownLatch latch = new CountDownLatch(1);
+                              Executors.newSingleThreadExecutor()
+                                  .execute(
+                                      () -> {
+                                        c.getProvider(Double.class);
+                                        latch.countDown();
+                                      });
+                              try {
+                                latch.await();
+                              } catch (InterruptedException e) {
+                                Thread.currentThread().interrupt();
+                                throw new AssertionError("Deadlock detected", e);
+                              }
+                              return "Hello";
+                            })
+                        .build()));
+
+    runtime.discoverComponents();
+    stringDeferredInjected.await();
+  }
+}
diff --git a/firebase-components/src/test/java/com/google/firebase/components/threading/EagerComponent.java b/firebase-components/src/test/java/com/google/firebase/components/threading/EagerComponent.java
@@ -0,0 +1,17 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.components.threading;
+
+public class EagerComponent {}
