Fix issues with recursive loading

core/src/com/google/inject/internal/FailableCache.java

@@ -16,65 +16,103 @@
 
 package com.google.inject.internal;
 
-import com.google.common.cache.CacheBuilder;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Maps;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.FutureTask;
 
 /**
  * Lazily creates (and caches) values for keys. If creating the value fails (with errors), an
  * exception is thrown on retrieval.
  *
+ * This defends against re-entrant loads, and removals of elements while they are being loaded.
+ *
  * @author jessewilson@google.com (Jesse Wilson)
  */
 public abstract class FailableCache<K, V> {
 
-  private final LoadingCache<K, Object> delegate =
-      CacheBuilder.newBuilder()
-          .build(
-              new CacheLoader<K, Object>() {
-                @Override
-                public Object load(K key) {
-                  Errors errors = new Errors();
-                  V result = null;
-                  try {
-                    result = FailableCache.this.create(key, errors);
-                  } catch (ErrorsException e) {
-                    errors.merge(e.getErrors());
-                  }
-                  return errors.hasErrors() ? errors : result;
-                }
-              });
+  private final Map<K, FutureTask<V>> delegate = new LinkedHashMap<>();
+
+  private final ThreadLocal<Set<K>> computingThreadLocal = ThreadLocal.withInitial(HashSet::new);
 
   protected abstract V create(K key, Errors errors) throws ErrorsException;
 
   public V get(K key, Errors errors) throws ErrorsException {
-    Object resultOrError = delegate.getUnchecked(key);
-    if (resultOrError instanceof Errors) {
-      errors.merge((Errors) resultOrError);
+    Set<K> computing = computingThreadLocal.get();
+    if (!computing.add(key)) {
+      errors.addMessage("Recursive load of %s", key);
+      throw errors.toException();
+    }
+
+    V result = null;
+    try {
+      FutureTask<V> futureTask;
+      synchronized (delegate) {
+        futureTask = delegate.computeIfAbsent(key, this::newFutureTask);
+      }
+
+      futureTask.run();
+      result = futureTask.get();
+    } catch (InterruptedException e) {
+      throw new RuntimeException(e);
+    } catch (ExecutionException e) {
+      Throwable cause = e.getCause();
+      if (cause instanceof ErrorsException) {
+        errors.merge(((ErrorsException) cause).getErrors());
+      } else {
+        throw new RuntimeException(e);
+      }
+    } finally {
+      computing.remove(key);
+    }
+
+    if (errors.hasErrors()) {
       throw errors.toException();
-    } else {
-      @SuppressWarnings("unchecked") // create returned a non-error result, so this is safe
-      V result = (V) resultOrError;
-      return result;
     }
+
+    return result;
+  }
+
+  private FutureTask<V> newFutureTask(K key) {
+    return new FutureTask<>(() -> {
+      Errors errors = new Errors();
+      V result = null;
+      try {
+        result = create(key, errors);
+      } catch (ErrorsException e) {
+        errors.merge(e.getErrors());
+      }
+      if (errors.hasErrors()) throw errors.toException();
+      return result;
+    });
   }
 
   boolean remove(K key) {
-    return delegate.asMap().remove(key) != null;
+    Set<K> computing = computingThreadLocal.get();
+    if (computing.contains(key)) {
+      return false; // Don't remove a value that's still being computed.
+    }
+    synchronized (delegate) {
+      return delegate.remove(key) != null;
+    }
   }
 
   Map<K, V> asMap() {
-    return Maps.transformValues(
-        Maps.filterValues(
-            ImmutableMap.copyOf(delegate.asMap()),
-            resultOrError -> !(resultOrError instanceof Errors)),
-        resultOrError -> {
-          @SuppressWarnings("unchecked") // create returned a non-error result, so this is safe
-          V result = (V) resultOrError;
-          return result;
-        });
+    synchronized (delegate) {
+      ImmutableMap.Builder<K, V> result = ImmutableMap.builder();
+      for (Map.Entry<K, FutureTask<V>> entry : delegate.entrySet()) {
+        FutureTask<V> futureTask = entry.getValue();
+        try {
+          if (futureTask.isDone()) {
+            result.put(entry.getKey(), futureTask.get());
+          }
+        } catch (Exception ignored) {
+        }
+      }
+      return result.build();
+    }
   }
 }

core/test/com/google/inject/RecursiveLoadTest.java

@@ -0,0 +1,106 @@
+/*
+ * Copyright (C) 2020 Google Inc.
+ *
+ * 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.inject;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+import static com.google.inject.Asserts.assertContains;
+import static org.junit.Assert.fail;
+
+@RunWith(JUnit4.class)
+public final class RecursiveLoadTest {
+  /**
+   * This test uses failed optional bindings to trigger a recursive load crash.
+   * https://github.com/google/guice/issues/785
+   */
+  @Test public void recursiveLoadWithOptionals() {
+    try {
+      Guice.createInjector(new AbstractModule() {
+        @Override protected void configure() {
+          bind(A.class);
+        }
+      });
+      fail();
+    } catch (CreationException expected) {
+      assertContains(
+          expected.getMessage(),
+          "Recursive load of " + B.class.getName() + ".<init>()");
+    }
+  }
+
+  static class A {
+    @Inject B b;
+  }
+
+  static class B {
+    @Inject C c;
+  }
+
+  static class C {
+    @Inject(optional = true) D d;
+    @Inject E e;
+  }
+
+  static class D {
+    @Inject B b;
+    @Inject Unresolved unresolved;
+  }
+
+  static class E {
+    @Inject B b;
+  }
+
+  @Test public void recursiveLoadWithoutOptionals() {
+    try {
+      Guice.createInjector(new AbstractModule() {
+        @Provides public V provideV(Z z) {
+          return null;
+        }
+      });
+      fail();
+    } catch (CreationException expected) {
+      assertContains(
+          expected.getMessage(),
+          "1) Recursive load of " + Z.class.getName() + ".<init>()");
+    }
+  }
+
+  static class V {
+  }
+
+  static class W {
+    @Inject Y y;
+    @Inject Z z;
+  }
+
+  static class X {
+    @Inject Z z;
+  }
+
+  static class Y {
+    @Inject Unresolved unresolved;
+  }
+
+  static class Z {
+    @Inject W w;
+    @Inject X x;
+  }
+
+  interface Unresolved {
+  }
+}