diff --git a/core/src/com/google/inject/internal/AbstractBindingProcessor.java b/core/src/com/google/inject/internal/AbstractBindingProcessor.java index c91319a765..e2c04d9738 100644 --- a/core/src/com/google/inject/internal/AbstractBindingProcessor.java +++ b/core/src/com/google/inject/internal/AbstractBindingProcessor.java @@ -97,7 +97,7 @@ protected void putBinding(BindingImpl binding) { } // prevent the parent from creating a JIT binding for this key - injector.state.parent().blacklist(key, injector.state, binding.getSource()); + injector.getJitBindingData().banKeyInParent(key, injector.state, binding.getSource()); injector.state.putBinding(key, binding); } diff --git a/core/src/com/google/inject/internal/InheritingState.java b/core/src/com/google/inject/internal/InheritingState.java index 7b132cf087..4b4424294d 100644 --- a/core/src/com/google/inject/internal/InheritingState.java +++ b/core/src/com/google/inject/internal/InheritingState.java @@ -64,13 +64,11 @@ final class InheritingState implements State { private final List typeListenerBindings = Lists.newArrayList(); private final List provisionListenerBindings = Lists.newArrayList(); private final List scannerBindings = Lists.newArrayList(); - private final WeakKeySet blacklistedKeys; private final Object lock; InheritingState(State parent) { this.parent = checkNotNull(parent, "parent"); this.lock = (parent == State.NONE) ? this : parent.lock(); - this.blacklistedKeys = new WeakKeySet(lock); } @Override @@ -253,22 +251,6 @@ public List getScannerBindingsThisLevel() { return scannerBindings; } - @Override - public void blacklist(Key key, State state, Object source) { - parent.blacklist(key, state, source); - blacklistedKeys.add(key, state, source); - } - - @Override - public boolean isBlacklisted(Key key) { - return blacklistedKeys.contains(key); - } - - @Override - public Set getSourcesForBlacklistedKey(Key key) { - return blacklistedKeys.getSources(key); - } - @Override public Object lock() { return lock; diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java index 78f717b90f..551baac6a4 100644 --- a/core/src/com/google/inject/internal/InjectorImpl.java +++ b/core/src/com/google/inject/internal/InjectorImpl.java @@ -26,7 +26,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; -import com.google.common.collect.Maps; import com.google.common.collect.Multimaps; import com.google.common.collect.Sets; import com.google.inject.Binder; @@ -116,26 +115,24 @@ enum JitLimitation { } final State state; + private final InjectorJitBindingData jitBindingData; final InjectorImpl parent; final ListMultimap, Binding> bindingsMultimap = ArrayListMultimap.create(); final InjectorOptions options; - /** Just-in-time binding cache. Guarded by state.lock() */ - final Map, BindingImpl> jitBindings = Maps.newHashMap(); - /** - * Cache of Keys that we were unable to create JIT bindings for, so we don't keep trying. Also - * guarded by state.lock(). - */ - final Set> failedJitBindings = Sets.newHashSet(); - Lookups lookups = new DeferredLookups(this); /** The set of types passed to {@link #getMembersInjector} and {@link #injectMembers}. */ final Set> userRequestedMembersInjectorTypes = Sets.newConcurrentHashSet(); - InjectorImpl(InjectorImpl parent, State state, InjectorOptions injectorOptions) { + InjectorImpl( + InjectorImpl parent, + State state, + InjectorJitBindingData jitBindingData, + InjectorOptions injectorOptions) { this.parent = parent; this.state = state; + this.jitBindingData = jitBindingData; this.options = injectorOptions; if (parent != null) { @@ -186,7 +183,8 @@ public BindingImpl getExistingBinding(Key key) { // See if any jit bindings have been created for this key. for (InjectorImpl injector = this; injector != null; injector = injector.parent) { @SuppressWarnings("unchecked") - BindingImpl jitBinding = (BindingImpl) injector.jitBindings.get(key); + BindingImpl jitBinding = + (BindingImpl) injector.jitBindingData.getJitBindings().get(key); if (jitBinding != null) { return jitBinding; } @@ -250,6 +248,10 @@ public Injector createChildInjector(Module... modules) { return createChildInjector(ImmutableList.copyOf(modules)); } + InjectorJitBindingData getJitBindingData() { + return jitBindingData; + } + /** * Returns a just-in-time binding for {@code key}, creating it if necessary. * @@ -263,7 +265,7 @@ private BindingImpl getJustInTimeBinding(Key key, Errors errors, JitLi // first try to find a JIT binding that we've already created for (InjectorImpl injector = this; injector != null; injector = injector.parent) { @SuppressWarnings("unchecked") // we only store bindings that match their key - BindingImpl binding = (BindingImpl) injector.jitBindings.get(key); + BindingImpl binding = (BindingImpl) injector.jitBindingData.getJitBindings().get(key); if (binding != null) { // If we found a JIT binding and we don't allow them, @@ -285,15 +287,15 @@ private BindingImpl getJustInTimeBinding(Key key, Errors errors, JitLi // entry in jitBindings (during cleanup), and we may be trying // to create it again (in the case of a recursive JIT binding). // We need both of these guards for different reasons - // failedJitBindings.contains: We want to continue processing if we've never + // getFailedJitBindings.contains: We want to continue processing if we've never // failed before, so that our initial error message contains // as much useful information as possible about what errors exist. // errors.hasErrors: If we haven't already failed, then it's OK to // continue processing, to make sure the ultimate error message // is the correct one. // See: ImplicitBindingsTest#testRecursiveJitBindingsCleanupCorrectly - // for where this guard compes into play. - if (failedJitBindings.contains(key) && errors.hasErrors()) { + // for where this guard comes into play. + if (jitBindingData.getFailedJitBindings().contains(key) && errors.hasErrors()) { throw errors.toException(); } return createJustInTimeBindingRecursive(key, errors, options.jitDisabled, jitType); @@ -598,7 +600,7 @@ void initializeJitBinding(BindingImpl binding, Errors errors) throws Erro // Note: We don't need to synchronize on state.lock() during injector creation. if (binding instanceof DelayedInitialize) { Key key = binding.getKey(); - jitBindings.put(key, binding); + jitBindingData.getJitBindings().put(key, binding); boolean successful = false; DelayedInitialize delayed = (DelayedInitialize) binding; try { @@ -629,7 +631,7 @@ private boolean cleanup(BindingImpl binding, Set encountered) { Key depKey = dep.getKey(); InjectionPoint ip = dep.getInjectionPoint(); if (encountered.add(depKey)) { // only check if we haven't looked at this key yet - BindingImpl depBinding = jitBindings.get(depKey); + BindingImpl depBinding = jitBindingData.getJitBindings().get(depKey); if (depBinding != null) { // if the binding still exists, validate boolean failed = cleanup(depBinding, encountered); // if children fail, we fail if (depBinding instanceof ConstructorBindingImpl) { @@ -655,8 +657,8 @@ private boolean cleanup(BindingImpl binding, Set encountered) { /** Cleans up any state that may have been cached when constructing the JIT binding. */ private void removeFailedJitBinding(Binding binding, InjectionPoint ip) { - failedJitBindings.add(binding.getKey()); - jitBindings.remove(binding.getKey()); + jitBindingData.getFailedJitBindings().add(binding.getKey()); + jitBindingData.getJitBindings().remove(binding.getKey()); membersInjectorStore.remove(binding.getKey().getTypeLiteral()); provisionListenerStore.remove(binding); if (ip != null) { @@ -846,22 +848,23 @@ private BindingImpl createJustInTimeBindingRecursive( jitDisabled, parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType); } catch (ErrorsException ignored) { + // TODO(b/160910914): Why are ErrorsExceptions ignored? } } - // Retrieve the sources before checking for blacklisting to guard against sources becoming null - // due to a full GC happening after calling state.isBlacklisted and - // state.getSourcesForBlacklistedKey. + // Retrieve the sources before checking for banned key to guard against sources becoming null + // due to a full GC happening after calling jitBindingData.isBanned and + // state.getSourcesForBannedKey. // TODO(user): Consolidate these two APIs. - Set sources = state.getSourcesForBlacklistedKey(key); - if (state.isBlacklisted(key)) { + Set sources = jitBindingData.getSourcesForBannedKey(key); + if (jitBindingData.isBannedKey(key)) { throw errors.childBindingAlreadySet(key, sources).toException(); } key = MoreTypes.canonicalizeKey(key); // before storing the key long-term, canonicalize it. BindingImpl binding = createJustInTimeBinding(key, errors, jitDisabled, jitType); - state.parent().blacklist(key, state, binding.getSource()); - jitBindings.put(key, binding); + jitBindingData.banKeyInParent(key, state, binding.getSource()); + jitBindingData.getJitBindings().put(key, binding); return binding; } @@ -884,12 +887,12 @@ private BindingImpl createJustInTimeBinding( throws ErrorsException { int numErrorsBefore = errors.size(); - // Retrieve the sources before checking for blacklisting to guard against sources becoming null - // due to a full GC happening after calling state.isBlacklisted and - // state.getSourcesForBlacklistedKey. + // Retrieve the sources before checking for a banned key to guard against sources becoming null + // due to a full GC happening after calling jitBindingData.isBanned and + // jitBindingData.getSourcesForBannedKey. // TODO(user): Consolidate these two APIs. - Set sources = state.getSourcesForBlacklistedKey(key); - if (state.isBlacklisted(key)) { + Set sources = jitBindingData.getSourcesForBannedKey(key); + if (jitBindingData.isBannedKey(key)) { throw errors.childBindingAlreadySet(key, sources).toException(); } @@ -958,7 +961,7 @@ public Map, Binding> getAllBindings() { synchronized (state.lock()) { return new ImmutableMap.Builder, Binding>() .putAll(state.getExplicitBindingsThisLevel()) - .putAll(jitBindings) + .putAll(jitBindingData.getJitBindings()) .build(); } } diff --git a/core/src/com/google/inject/internal/InjectorJitBindingData.java b/core/src/com/google/inject/internal/InjectorJitBindingData.java new file mode 100644 index 0000000000..296b490a68 --- /dev/null +++ b/core/src/com/google/inject/internal/InjectorJitBindingData.java @@ -0,0 +1,87 @@ +package com.google.inject.internal; + +import com.google.common.collect.Maps; +import com.google.common.collect.Sets; +import com.google.inject.Key; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +/** A container for just-in-time (JIT) binding data corresponding to an Injector. */ +final class InjectorJitBindingData { + // TODO(b/159459925): the State object is currently used as the lock for accessing all the JIT + // binding data fields. Migrate to using InjectorJitBindingData as a lock for these accesses + // instead. + // TODO(b/159459925): Hide direct access to internal jitBindings and failedJitBindings fields once + // locks are managed by this class + /** Just-in-time binding cache. Guarded by state.lock() */ + private final Map, BindingImpl> jitBindings = Maps.newHashMap(); + /** + * Cache of Keys that we were unable to create JIT bindings for, so we don't keep trying. Also + * guarded by state.lock(). + */ + private final Set> failedJitBindings = Sets.newHashSet(); + + // The set of JIT binding keys that are banned for this particular injector, because a binding + // already exists in a child injector. + private final WeakKeySet bannedKeys; + + // The InjectorJitBindingData corresponding to the Injector's parent, if it exists. + private final Optional parent; + + InjectorJitBindingData(Optional parent, State state) { + this.parent = parent; + this.bannedKeys = new WeakKeySet(state.lock()); + } + + /** + * Returns a mutable map containing the JIT bindings for the injector. Accesses to this need to be + * guarded by state.lock(). + */ + Map, BindingImpl> getJitBindings() { + return jitBindings; + } + + /** + * Returns a mutable set containing the failed JIT bindings for the injector. Accesses to this + * need to be guarded by state.lock(). + */ + Set> getFailedJitBindings() { + return failedJitBindings; + } + + /** + * Forbids the corresponding injector and its ancestors from creating a binding to {@code key}. + * Child injectors ban their bound keys on their parent injectors to prevent just-in-time bindings + * on the parent injector that would conflict, and pass along their State to control the banned + * key's lifetime. + */ + void banKey(Key key, State state, Object source) { + banKeyInParent(key, state, source); + bannedKeys.add(key, state, source); + } + + /** + * Similar to {@link #banKey(Key, State, Object)} but we only begin banning the binding at the + * parent level. This is used to prevent JIT bindings in the parent injector from overriding + * explicit bindings declared in a child injector. + */ + void banKeyInParent(Key key, State state, Object source) { + if (parent.isPresent()) { + parent.get().banKey(key, state, source); + } + } + + /** + * Returns true if {@code key} is forbidden from being bound in the injector corresponding to this + * data object. This indicates that one of the injector's children has bound the key. + */ + boolean isBannedKey(Key key) { + return bannedKeys.contains(key); + } + + /** Returns the source of a banned key. */ + Set getSourcesForBannedKey(Key key) { + return bannedKeys.getSources(key); + } +} diff --git a/core/src/com/google/inject/internal/InjectorShell.java b/core/src/com/google/inject/internal/InjectorShell.java index 2b4ee52268..8c9a0b8c63 100644 --- a/core/src/com/google/inject/internal/InjectorShell.java +++ b/core/src/com/google/inject/internal/InjectorShell.java @@ -42,6 +42,7 @@ import com.google.inject.spi.ProvisionListenerBinding; import com.google.inject.spi.TypeListenerBinding; import java.util.List; +import java.util.Optional; import java.util.logging.Logger; /** @@ -72,8 +73,9 @@ static class Builder { private final List elements = Lists.newArrayList(); private final List modules = Lists.newArrayList(); - /** lazily constructed */ + // lazily constructed fields private State state; + private InjectorJitBindingData jitBindingData; private InjectorImpl parent; private InjectorOptions options; @@ -90,6 +92,8 @@ Builder stage(Stage stage) { Builder parent(InjectorImpl parent) { this.parent = parent; this.state = new InheritingState(parent.state); + this.jitBindingData = + new InjectorJitBindingData(Optional.of(parent.getJitBindingData()), state); this.options = parent.options; this.stage = options.stage; return this; @@ -154,7 +158,7 @@ List build( optionsProcessor.process(null, elements); options = optionsProcessor.getOptions(stage, options); - InjectorImpl injector = new InjectorImpl(parent, state, options); + InjectorImpl injector = new InjectorImpl(parent, state, jitBindingData, options); if (privateElements != null) { privateElements.initInjector(injector); } @@ -216,9 +220,14 @@ List build( return injectorShells; } + /** + * Lazily initializes state and jitBindingData, if they were not already initialized with a + * parent injector by {@link #parent(InjectorImpl)}. + */ private State getState() { if (state == null) { state = new InheritingState(State.NONE); + jitBindingData = new InjectorJitBindingData(Optional.empty(), state); } return state; } diff --git a/core/src/com/google/inject/internal/InternalInjectorCreator.java b/core/src/com/google/inject/internal/InternalInjectorCreator.java index 85b5826322..e8f7c7c838 100644 --- a/core/src/com/google/inject/internal/InternalInjectorCreator.java +++ b/core/src/com/google/inject/internal/InternalInjectorCreator.java @@ -200,7 +200,7 @@ void loadEagerSingletons(InjectorImpl injector, Stage stage, final Errors errors candidateBindings.addAll(bindingsAtThisLevel); synchronized (injector.state.lock()) { // jit bindings must be accessed while holding the lock. - candidateBindings.addAll(injector.jitBindings.values()); + candidateBindings.addAll(injector.getJitBindingData().getJitBindings().values()); } InternalContext context = injector.enterContext(); try { diff --git a/core/src/com/google/inject/internal/State.java b/core/src/com/google/inject/internal/State.java index 48cc46888a..fda3f7df27 100644 --- a/core/src/com/google/inject/internal/State.java +++ b/core/src/com/google/inject/internal/State.java @@ -177,18 +177,7 @@ public List getScannerBindings() { return ImmutableList.of(); } - @Override - public void blacklist(Key key, State state, Object source) {} - - @Override - public boolean isBlacklisted(Key key) { - return true; - } - @Override - public Set getSourcesForBlacklistedKey(Key key) { - throw new UnsupportedOperationException(); - } @Override public Object lock() { @@ -286,22 +275,6 @@ TypeConverterBinding getConverter( List getScannerBindingsThisLevel(); - /** - * Forbids the corresponding injector from creating a binding to {@code key}. Child injectors - * blacklist their bound keys on their parent injectors to prevent just-in-time bindings on the - * parent injector that would conflict and pass along their state to control the lifetimes. - */ - void blacklist(Key key, State state, Object source); - - /** - * Returns true if {@code key} is forbidden from being bound in this injector. This indicates that - * one of this injector's descendent's has bound the key. - */ - boolean isBlacklisted(Key key); - - /** Returns the source of a blacklisted key. */ - Set getSourcesForBlacklistedKey(Key key); - /** * Returns the shared lock for all injector data. This is a low-granularity, high-contention lock * to be used when reading mutable data (ie. just-in-time bindings, and binding blacklists). diff --git a/core/test/com/google/inject/internal/WeakKeySetTest.java b/core/test/com/google/inject/internal/WeakKeySetTest.java index 7cbf8f3ede..06948b98b4 100644 --- a/core/test/com/google/inject/internal/WeakKeySetTest.java +++ b/core/test/com/google/inject/internal/WeakKeySetTest.java @@ -135,7 +135,8 @@ public void testEviction_keyOverlap_2x() { assertSourceNotInSet(set, key, source1); assertInSet(set, key, 1, source2); - source1 = source2 = null; + // Clear source1 and source2 fields so the objects can be GCed. + Object unused = source1 = source2 = null; awaitClear(weakSource1Ref); // Key1 will be referenced as the key in the sources backingSet and won't be @@ -585,19 +586,6 @@ public List getScannerBindings() { return ImmutableList.of(); } - @Override - public void blacklist(Key key, State state, Object source) {} - - @Override - public boolean isBlacklisted(Key key) { - return true; - } - - @Override - public Set getSourcesForBlacklistedKey(Key key) { - throw new UnsupportedOperationException(); - } - @Override public Object lock() { throw new UnsupportedOperationException(); diff --git a/core/test/com/google/inject/internal/WeakKeySetUtils.java b/core/test/com/google/inject/internal/WeakKeySetUtils.java index 80bc59b436..4ff3172bdf 100644 --- a/core/test/com/google/inject/internal/WeakKeySetUtils.java +++ b/core/test/com/google/inject/internal/WeakKeySetUtils.java @@ -81,13 +81,13 @@ private static void assertBlacklistState(Injector injector, Key key, boolean // if we're expecting it to not be blacklisted, loop around and wait for threads to run. if (!isBlacklisted) { for (int i = 0; i < 10; i++) { - if (!((InjectorImpl) injector).state.isBlacklisted(key)) { + if (!((InjectorImpl) injector).getJitBindingData().isBannedKey(key)) { break; } sleep(); } } - assertEquals(isBlacklisted, ((InjectorImpl) injector).state.isBlacklisted(key)); + assertEquals(isBlacklisted, ((InjectorImpl) injector).getJitBindingData().isBannedKey(key)); } private static void sleep() { @@ -96,6 +96,7 @@ private static void sleep() { } catch (InterruptedException e) { throw new RuntimeException(e); } + // TODO(b/160912368): fix the ThreadPriorityCheck errorprone warning on this line. Thread.yield(); } }