diff --git a/core/src/com/google/inject/internal/BindingProcessor.java b/core/src/com/google/inject/internal/BindingProcessor.java index b5e1a9f628..284fda3dc6 100644 --- a/core/src/com/google/inject/internal/BindingProcessor.java +++ b/core/src/com/google/inject/internal/BindingProcessor.java @@ -145,7 +145,8 @@ public Boolean visit(ProviderInstanceBinding binding) { new InternalFactoryToInitializableAdapter( initializable, source, - injector.provisionListenerStore.get((ProviderInstanceBinding) binding)); + injector.provisionListenerStore.get((ProviderInstanceBinding) binding), + injector.circularFactoryIdFactory.next()); InternalFactory scopedFactory = Scoping.scope(key, injector, factory, source, scoping); putBinding( diff --git a/core/src/com/google/inject/internal/BoundProviderFactory.java b/core/src/com/google/inject/internal/BoundProviderFactory.java index 01d220b8e4..32ccb23d65 100644 --- a/core/src/com/google/inject/internal/BoundProviderFactory.java +++ b/core/src/com/google/inject/internal/BoundProviderFactory.java @@ -19,7 +19,6 @@ import com.google.inject.Key; import com.google.inject.internal.InjectorImpl.JitLimitation; import com.google.inject.spi.Dependency; -import jakarta.inject.Provider; /** Delegates to a custom factory which is also bound in the injector. */ final class BoundProviderFactory extends ProviderInternalFactory implements CreationListener { @@ -34,7 +33,7 @@ final class BoundProviderFactory extends ProviderInternalFactory implement Key> providerKey, Object source, ProvisionListenerStackCallback provisionCallback) { - super(source); + super(source, injector.circularFactoryIdFactory.next()); this.provisionCallback = provisionCallback; this.injector = injector; this.providerKey = providerKey; @@ -62,19 +61,6 @@ public T get(InternalContext context, Dependency dependency, boolean linked) } } - @Override - protected T provision( - Provider provider, - Dependency dependency, - ConstructionContext constructionContext) - throws InternalProvisionException { - try { - return super.provision(provider, dependency, constructionContext); - } catch (RuntimeException userException) { - throw InternalProvisionException.errorInProvider(userException); - } - } - @Override public String toString() { return providerKey.toString(); diff --git a/core/src/com/google/inject/internal/ConstructionContext.java b/core/src/com/google/inject/internal/ConstructionContext.java deleted file mode 100644 index 98eb405d98..0000000000 --- a/core/src/com/google/inject/internal/ConstructionContext.java +++ /dev/null @@ -1,91 +0,0 @@ -/* - * Copyright (C) 2006 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.internal; - -import com.google.inject.internal.InjectorImpl.InjectorOptions; -import java.util.ArrayList; -import java.util.List; - -/** - * Context of a dependency construction. Used to manage circular references. - * - * @author crazybob@google.com (Bob Lee) - */ -final class ConstructionContext { - - T currentReference; - boolean constructing; - - List> invocationHandlers; - - public T getCurrentReference() { - return currentReference; - } - - public void removeCurrentReference() { - this.currentReference = null; - } - - public void setCurrentReference(T currentReference) { - this.currentReference = currentReference; - } - - public boolean isConstructing() { - return constructing; - } - - public void startConstruction() { - this.constructing = true; - } - - public void finishConstruction() { - this.constructing = false; - invocationHandlers = null; - } - - public Object createProxy(InjectorOptions injectorOptions, Class expectedType) - throws InternalProvisionException { - if (injectorOptions.disableCircularProxies) { - throw InternalProvisionException.circularDependenciesDisabled(expectedType); - } - if (!expectedType.isInterface()) { - throw InternalProvisionException.cannotProxyClass(expectedType); - } - - if (invocationHandlers == null) { - invocationHandlers = new ArrayList<>(); - } - - DelegatingInvocationHandler invocationHandler = new DelegatingInvocationHandler<>(); - invocationHandlers.add(invocationHandler); - - // TODO: if I create a proxy which implements all the interfaces of - // the implementation type, I'll be able to get away with one proxy - // instance (as opposed to one per caller). - return BytecodeGen.newCircularProxy(expectedType, invocationHandler); - } - - public void setProxyDelegates(T delegate) { - if (invocationHandlers != null) { - for (DelegatingInvocationHandler handler : invocationHandlers) { - handler.setDelegate(delegate); - } - // initialization of each handler can happen no more than once - invocationHandlers = null; - } - } -} diff --git a/core/src/com/google/inject/internal/ConstructorBindingImpl.java b/core/src/com/google/inject/internal/ConstructorBindingImpl.java index 97fdae097c..e5819f4611 100644 --- a/core/src/com/google/inject/internal/ConstructorBindingImpl.java +++ b/core/src/com/google/inject/internal/ConstructorBindingImpl.java @@ -73,7 +73,12 @@ public ConstructorBindingImpl( new DefaultConstructionProxyFactory(constructorInjectionPoint).create(); this.constructorInjectionPoint = constructorInjectionPoint; factory.constructorInjector = - new ConstructorInjector(injectionPoints, constructionProxy, null, null); + new ConstructorInjector( + injectionPoints, + constructionProxy, + null, + null, + /* circularFactoryId= */ InternalContext.CircularFactoryIdFactory.INVALID_ID); } /** diff --git a/core/src/com/google/inject/internal/ConstructorInjector.java b/core/src/com/google/inject/internal/ConstructorInjector.java index 440fc9ef61..2f61d3f16c 100644 --- a/core/src/com/google/inject/internal/ConstructorInjector.java +++ b/core/src/com/google/inject/internal/ConstructorInjector.java @@ -35,17 +35,21 @@ final class ConstructorInjector { private final ImmutableSet injectableMembers; private final SingleParameterInjector[] parameterInjectors; private final ConstructionProxy constructionProxy; - private final MembersInjectorImpl membersInjector; + @Nullable private final MembersInjectorImpl membersInjector; + private final int circularFactoryId; ConstructorInjector( Set injectableMembers, ConstructionProxy constructionProxy, SingleParameterInjector[] parameterInjectors, - MembersInjectorImpl membersInjector) { + @Nullable MembersInjectorImpl membersInjector, + int circularFactoryId) { this.injectableMembers = ImmutableSet.copyOf(injectableMembers); this.constructionProxy = constructionProxy; this.parameterInjectors = parameterInjectors; - this.membersInjector = membersInjector; + this.membersInjector = + membersInjector == null || membersInjector.isEmpty() ? null : membersInjector; + this.circularFactoryId = circularFactoryId; } public ImmutableSet getInjectableMembers() { @@ -65,64 +69,50 @@ Object construct( Dependency dependency, @Nullable ProvisionListenerStackCallback provisionCallback) throws InternalProvisionException { - final ConstructionContext constructionContext = context.getConstructionContext(this); - // We have a circular reference between constructors. Return a proxy. - if (constructionContext.isConstructing()) { - // TODO (user): if we can't proxy this object, can we proxy the other object? - return constructionContext.createProxy( - context.getInjectorOptions(), dependency.getKey().getTypeLiteral().getRawType()); + @SuppressWarnings("unchecked") + T result = (T) context.tryStartConstruction(circularFactoryId, dependency); + if (result != null) { + // We have a circular reference between bindings. Return a proxy. + return result; } - // If we're re-entering this factory while injecting fields or methods, - // return the same instance. This prevents infinite loops. - T t = constructionContext.getCurrentReference(); - if (t != null) { - if (context.getInjectorOptions().disableCircularProxies) { - throw InternalProvisionException.circularDependenciesDisabled( - dependency.getKey().getTypeLiteral().getRawType()); - } - return t; - } - - constructionContext.startConstruction(); - try { - // Optimization: Don't go through the callback stack if we have no listeners. - if (provisionCallback == null) { - return provision(context, constructionContext); - } else { - return provisionCallback.provision( - context, - new ProvisionCallback() { - @Override - public T call() throws InternalProvisionException { - return provision(context, constructionContext); - } - }); - } - } finally { - constructionContext.finishConstruction(); + // Optimization: Don't go through the callback stack if we have no listeners. + if (provisionCallback == null) { + return provision(context); + } else { + // NOTE: `provision` always calls the callback, even if provision listeners + // throw exceptions. + return provisionCallback.provision( + context, + new ProvisionCallback() { + @Override + public T call() throws InternalProvisionException { + return provision(context); + } + }); } } /** Provisions a new T. */ - private T provision(InternalContext context, ConstructionContext constructionContext) - throws InternalProvisionException { + private T provision(InternalContext context) throws InternalProvisionException { + MembersInjectorImpl localMembersInjector = membersInjector; try { - T t; + T t = null; try { Object[] parameters = SingleParameterInjector.getAll(context, parameterInjectors); t = constructionProxy.newInstance(parameters); - constructionContext.setProxyDelegates(t); } finally { - constructionContext.finishConstruction(); + if (localMembersInjector == null) { + context.finishConstruction(circularFactoryId, t); + } else { + context.finishConstructionAndSetReference(circularFactoryId, t); + } } - // Store reference. If an injector re-enters this factory, they'll get the same reference. - constructionContext.setCurrentReference(t); - - MembersInjectorImpl localMembersInjector = membersInjector; - localMembersInjector.injectMembers(t, context, false); - localMembersInjector.notifyListeners(t); + if (localMembersInjector != null) { + localMembersInjector.injectMembers(t, context, /* toolableOnly= */ false); + localMembersInjector.notifyListeners(t); + } return t; } catch (InvocationTargetException userException) { @@ -130,7 +120,9 @@ private T provision(InternalContext context, ConstructionContext construction throw InternalProvisionException.errorInjectingConstructor(cause) .addSource(constructionProxy.getInjectionPoint()); } finally { - constructionContext.removeCurrentReference(); + if (localMembersInjector != null) { + context.clearCurrentReference(circularFactoryId); + } } } } diff --git a/core/src/com/google/inject/internal/ConstructorInjectorStore.java b/core/src/com/google/inject/internal/ConstructorInjectorStore.java index 8e650d9519..4ef9af8d4e 100644 --- a/core/src/com/google/inject/internal/ConstructorInjectorStore.java +++ b/core/src/com/google/inject/internal/ConstructorInjectorStore.java @@ -97,6 +97,7 @@ private ConstructorInjector createConstructor(InjectionPoint injectionPoi membersInjector.getInjectionPoints(), factory.create(), constructorParameterInjectors, - membersInjector); + membersInjector, + injector.circularFactoryIdFactory.next()); } } diff --git a/core/src/com/google/inject/internal/FactoryProxy.java b/core/src/com/google/inject/internal/FactoryProxy.java index 81c63a6d6d..293bab6327 100644 --- a/core/src/com/google/inject/internal/FactoryProxy.java +++ b/core/src/com/google/inject/internal/FactoryProxy.java @@ -55,11 +55,10 @@ public void notify(final Errors errors) { @Override public T get(InternalContext context, Dependency dependency, boolean linked) throws InternalProvisionException { - Key localTargetKey = targetKey; try { - return targetFactory.get(context, dependency, true); + return targetFactory.get(context, dependency, /* linked= */ true); } catch (InternalProvisionException ipe) { - throw ipe.addSource(localTargetKey); + throw ipe.addSource(targetKey); } } diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java index 25a3dc74c0..40d3addcca 100644 --- a/core/src/com/google/inject/internal/InjectorImpl.java +++ b/core/src/com/google/inject/internal/InjectorImpl.java @@ -118,6 +118,7 @@ enum JitLimitation { private final InjectorJitBindingData jitBindingData; final InjectorImpl parent; final InjectorOptions options; + final InternalContext.CircularFactoryIdFactory circularFactoryIdFactory; Lookups lookups = new DeferredLookups(this); @@ -136,11 +137,13 @@ enum JitLimitation { if (parent != null) { localContext = parent.localContext; + circularFactoryIdFactory = parent.circularFactoryIdFactory; } else { // No ThreadLocal.initialValue(), as that would cause classloader leaks. See // https://github.com/google/guice/issues/288#issuecomment-48216933, // https://github.com/google/guice/issues/288#issuecomment-48216944 localContext = new ThreadLocal<>(); + circularFactoryIdFactory = new InternalContext.CircularFactoryIdFactory(); } } @@ -192,7 +195,7 @@ public BindingImpl getExistingBinding(Key key) { if (isProvider(key)) { try { // This is safe because isProvider above ensures that T is a Provider - @SuppressWarnings({"unchecked", "cast"}) + @SuppressWarnings("unchecked") Key providedKey = (Key) getProvidedKey((Key) key, new Errors()); if (getExistingBinding(providedKey) != null) { return getBinding(key); @@ -683,7 +686,7 @@ private void removeFailedJitBinding(Binding binding, InjectionPoint ip) { /** Safely gets the dependencies of possibly not initialized bindings. */ private Set> getInternalDependencies(BindingImpl binding) { if (binding instanceof ConstructorBindingImpl) { - return ((ConstructorBindingImpl) binding).getInternalDependencies(); + return ((ConstructorBindingImpl) binding).getInternalDependencies(); } else if (binding instanceof HasDependencies) { return ((HasDependencies) binding).getDependencies(); } else { @@ -807,7 +810,8 @@ BindingImpl createProvidedByBinding( @SuppressWarnings("unchecked") Key> providerKey = (Key>) Key.get(providerType); ProvidedByInternalFactory internalFactory = - new ProvidedByInternalFactory(rawType, providerType, providerKey); + new ProvidedByInternalFactory( + rawType, providerType, providerKey, circularFactoryIdFactory.next()); Object source = rawType; BindingImpl binding = LinkedProviderBindingImpl.createWithInitializer( @@ -937,7 +941,7 @@ private BindingImpl createJustInTimeBinding( if (isProvider(key)) { // These casts are safe. We know T extends Provider and that given Key>, // createSyntheticProviderBinding() will return BindingImpl>. - @SuppressWarnings({"unchecked", "cast"}) + @SuppressWarnings("unchecked") BindingImpl binding = (BindingImpl) createSyntheticProviderBinding((Key) key, errors); return binding; } @@ -946,7 +950,7 @@ private BindingImpl createJustInTimeBinding( if (isMembersInjector(key)) { // These casts are safe. T extends MembersInjector and that given Key>, // createMembersInjectorBinding() will return BindingImpl>. - @SuppressWarnings({"unchecked", "cast"}) + @SuppressWarnings("unchecked") BindingImpl binding = (BindingImpl) createMembersInjectorBinding((Key) key, errors); return binding; } @@ -1228,7 +1232,7 @@ InternalContext enterContext() { } InternalContext ctx = (InternalContext) reference[0]; if (ctx == null) { - reference[0] = ctx = new InternalContext(options, reference); + reference[0] = ctx = InternalContext.create(options.disableCircularProxies, reference); } else { ctx.enter(); } diff --git a/core/src/com/google/inject/internal/InternalContext.java b/core/src/com/google/inject/internal/InternalContext.java index 2d408b9992..1b8de696b4 100644 --- a/core/src/com/google/inject/internal/InternalContext.java +++ b/core/src/com/google/inject/internal/InternalContext.java @@ -16,28 +16,87 @@ package com.google.inject.internal; -import com.google.inject.internal.InjectorImpl.InjectorOptions; +import com.google.common.annotations.VisibleForTesting; import com.google.inject.spi.Dependency; -import java.util.IdentityHashMap; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.VarHandle; +import java.util.ArrayList; +import javax.annotation.Nullable; /** * Internal context. Used to coordinate injections and support circular dependencies. * * @author crazybob@google.com (Bob Lee) */ -final class InternalContext implements AutoCloseable { +abstract class InternalContext implements AutoCloseable { - private final InjectorOptions options; + /** A factory for generating unique circular factory ids. */ + static final class CircularFactoryIdFactory { + private static final VarHandle ID_HANDLE; - private final IdentityHashMap> constructionContexts = - new IdentityHashMap<>(); + static { + try { + ID_HANDLE = + MethodHandles.lookup().findVarHandle(CircularFactoryIdFactory.class, "id", int.class); + } catch (ReflectiveOperationException e) { + throw new LinkageError("Failed to find id field", e); + } + } + + static final int INVALID_ID = 0; + + // The first factory id is 1 so that we never produce a circular factory id of 0. + // The tables below use 0 to represent an empty slot which is convenient since arrays are always + // zero initialized. + @SuppressWarnings({"FieldCanBeFinal", "unused"}) + private int id = 1; + + /** Returns the next circular factory id. */ + int next() { + // Using an increment of 1 will produce an overflow after 2^32 - 1 calls to next() which is + // maximal for this type, however creates a simple way to reason about hash collisions. + // First we should note that the tables below use robin hood hashing and integer keys so + // collisions are relatively cheap. + // Our tables also use a trivial power of 2 size open addressed hash table, so sequentially + // allocated factories will never collide (since they differ in the lowest bits), instead we + // only expect collisions when the factories are some power of 2 > 4 apart. The order of + // factory allocation is difficult to reason about given the complexity of injector + // construction but we should generally expect that factories with dependency relationships + // (implying that they will both be constructing at the same time), will be allocated close to + // each other and thus are unlikely to collide. + while (true) { + // NOTE: we could use `getAndIncrement` but that would only allow us to throw an exception + // on the first time we overflow, this approach ensures that overflow completely breaks the + // factory. + // Use opaque reads since we don't expect much contention and don't need full barriers + // for correctness. + int next = (int) ID_HANDLE.getOpaque(this); + if (next == 0) { + // If this ever happens we can switch to using `long` values. + throw new IllegalStateException("Circular factory id overflow"); + } + if (ID_HANDLE.weakCompareAndSetPlain(this, next, next + 1)) { + return next; + } + } + } + } + + static InternalContext create(boolean disableCircularProxies, Object[] toClear) { + return disableCircularProxies + ? new WithoutProxySupport(toClear) + : new WithProxySupport(toClear); + } + + // enough space for 12 values before we need to resize the table + private static final int INITIAL_TABLE_SIZE = 16; /** Keeps track of the type that is currently being requested for injection. */ private Dependency dependency; /** * The number of times {@link #enter()} has been called + 1 for initial construction. This value - * is decremented when {@link #exit()} is called. + * is decremented when {@link #close()} is called. */ private int enterCount; @@ -48,8 +107,7 @@ final class InternalContext implements AutoCloseable { */ private final Object[] toClear; - InternalContext(InjectorOptions options, Object[] toClear) { - this.options = options; + protected InternalContext(Object[] toClear) { this.toClear = toClear; this.enterCount = 1; } @@ -71,21 +129,66 @@ public void close() { } } - InjectorOptions getInjectorOptions() { - return options; - } + /** + * Returns true if circular proxies are enabled. + * + *

This is only used by {@link SingletonScope} to determine if it should throw an exception or + * return a proxy when a circular dependency is detected. All other users get this behavior + * implicitly via the lifecycle methods in this class. + */ + abstract boolean areCircularProxiesEnabled(); - @SuppressWarnings("unchecked") - ConstructionContext getConstructionContext(Object key) { - ConstructionContext constructionContext = - (ConstructionContext) constructionContexts.get(key); - if (constructionContext == null) { - constructionContext = new ConstructionContext<>(); - constructionContexts.put(key, constructionContext); - } - return constructionContext; - } + /** + * Starts construction in the factory to satisfy the given dependency. + * + *

Following this call the user should call {@link #finishConstruction} or {@link + * #finishConstructionAndSetReference} to complete the construction. + * + * @return {@code null} if construction was started, or a proxy if a circular dependency was + * detected and created + * @throws InternalProvisionException if a circular dependency was detected and circular proxies + * are disabled + */ + @Nullable + abstract T tryStartConstruction(int circularFactoryId, Dependency forDependency) + throws InternalProvisionException; + + /** + * Marks construction complete for the given {@code circularFactoryId}, with the given result. + * + *

The {@code result} is used to satisfy any circular proxies that may have been created. + * + *

This is a terminal operation, the construction is considered complete and no state is + * retained. + */ + abstract void finishConstruction(int circularFactoryId, @Nullable Object result); + /** + * Finishes construction and sets the current reference to the given result, this is used for + * constructor injection where we proceed in 2 phases, constructor and then members. + * + *

The {@code result} is used to satisfy any circular proxies that may have been created and + * also any cycles detected during members injection. + * + *

This is a non-terminal operation, Following this call the user should call {@link + * #clearCurrentReference} to clear the current reference and finalize the construction. + */ + abstract void finishConstructionAndSetReference(int circularFactoryId, Object result); + + /** + * Completes construction and clears the current reference for constructor injection. + * + *

This is a terminal operation, the construction is considered complete and no state is + * retained. + */ + abstract void clearCurrentReference(int circularFactoryId); + + /** + * Returns the current dependency being injected. + * + *

This is only used by scope implementations to detect circular dependencies and to enforce + * nullability + */ Dependency getDependency() { return dependency; } @@ -99,4 +202,431 @@ Dependency getDependency() { void setDependency(Dependency dependency) { this.dependency = dependency; } + + @VisibleForTesting + static final class WithoutProxySupport extends InternalContext { + // Power of 2 size open addressed hash set using robin hood hashing, storing the circular + // factory ids. + private int[] table = new int[INITIAL_TABLE_SIZE]; + // The number of elements in the table, used to determine when to resize the table. + private int tableSize; + + WithoutProxySupport(Object[] toClear) { + super(toClear); + } + + @Override + boolean areCircularProxiesEnabled() { + return false; + } + + @Override + T tryStartConstruction(int circularFactoryId, Dependency forDependency) + throws InternalProvisionException { + insert(circularFactoryId, forDependency); + return null; + } + + @Override + void finishConstruction(int circularFactoryId, @Nullable Object result) { + remove(circularFactoryId); + } + + @Override + void finishConstructionAndSetReference(int circularFactoryId, Object result) { + // Do nothing. Leave the factory in the provisioning state since when circular proxies are + // disabled, we don't need to track the current reference. + // We could assert that the factory is in the provisioning state, but that would require + // checking the value of the table entry, which is expensive. + } + + @Override + void clearCurrentReference(int circularFactoryId) { + remove(circularFactoryId); + } + + /** + * Returns the index of where the given key should be inserted, or the index where it already is + * if present. To tell the difference, check the value in the construction contexts array. + */ + void insert(int key, Dependency forDependency) throws InternalProvisionException { + if (key == 0) { + throw new IllegalArgumentException("Invalid key: " + key); + } + int[] table = this.table; + int len = table.length; + int index = hash(key, len); + int c = table[index]; + int distance = 0; + while (true) { + if (c == 0) { + table[index] = key; + int newSize = ++this.tableSize; + if (newSize + (newSize >> 1) > len) { + expandTable(); + } + return; + } + if (c == key) { + throw InternalProvisionException.circularDependenciesDisabled( + forDependency.getKey().getTypeLiteral().getRawType()); + } + // See how far is the current key from its ideal slot in the table. + int cDistance = distance(c, index, len); + if (cDistance < distance) { + // Steal from the rich! + table[index] = key; + // Re-insert the key since we have stolen its slot. + insert(c, null); + return; + } + // Move to the next slot. + index = nextIndex(index, len); + distance++; + c = table[index]; + } + } + + // A testing util to get the index of a key. + @VisibleForTesting + int get(int key) { + int[] table = this.table; + int len = table.length; + int index = hash(key, len); + int c = table[index]; + int distance = 0; + while (true) { + if (c == 0) { + return -1; + } + if (c == key) { + return index; + } + int cDistance = distance(c, index, len); + if (cDistance < distance) { + return -1; + } + // Move to the next slot. + index = nextIndex(index, len); + distance++; + c = table[index]; + } + } + + /** + * Removes the given key from the table. + * + *

Returns the value associated with the key, throws if the key was not found. + */ + void remove(int key) { + int[] table = this.table; + int len = table.length; + int index = hash(key, len); + int c = table[index]; + int distance = 0; + while (true) { + if (c == 0) { + throw new IllegalStateException("table corrupted, key" + key + " not found"); + } + if (c == key) { + table[index] = 0; + // NOTE: We don't currently resize when removing items. + this.tableSize--; + // At this point we have potentially violated the robin hood invariant so we need to shift + // following items back. + // We shift each following item back by 1 unless it is empty or it is has a distance of + // zero. + int nextIndex = nextIndex(index, len); + int nextC = table[nextIndex]; + while (true) { + if (nextC == 0) { + break; // empty slot + } + if (hash(nextC, len) == nextIndex) { + break; // this item is in its ideal slot, so we cannot shift it + } + table[index] = nextC; + table[nextIndex] = 0; + index = nextIndex; + nextIndex = nextIndex(nextIndex, len); + nextC = table[nextIndex]; + } + return; + } + int cDistance = distance(c, index, len); + if (cDistance < distance) { + // This means that we have transitioned into another bucket chain. + throw new IllegalStateException("table corrupted, key " + key + " not found"); + } + index = nextIndex(index, len); + distance++; + c = table[index]; + } + } + + private void expandTable() throws InternalProvisionException { + int[] oldTable = this.table; + int oldLength = oldTable.length; + int newLength = oldLength * 2; + if (oldLength >= newLength) { // overflow + throw new IllegalArgumentException("Table size too large to expand: " + oldLength); + } + this.table = new int[newLength]; + this.tableSize = 0; + for (int k : oldTable) { + if (k != 0) { + insert(k, null); + } + } + } + } + + @VisibleForTesting + static final class WithProxySupport extends InternalContext { + // Need a custom private class to ensure we can tell the difference between this and a user + // constructed object. + private static final class ProxyDelegates + extends ArrayList> {} + + // Open addressed hash table, power of 2 size using robin hood hashing. + // Keys are the circular factory ids + // Values are in the constructionContexts table stored in parallel + private int[] table = new int[INITIAL_TABLE_SIZE]; + + // The list of construction contexts, parallel to table + // If the value is `null` but there is a key in the table then we are in the 'constructing' + // state + // If the value is not `null` then either it is a `ProxyDelegates` instance or a 'current + // reference' which might be any other type. + private Object[] constructionContexts = new Object[INITIAL_TABLE_SIZE]; + + // The number of elements in the table, used to determine when to resize the table. + private int tableSize; + + WithProxySupport(Object[] toClear) { + super(toClear); + } + + @Override + boolean areCircularProxiesEnabled() { + return true; + } + + @Override + T tryStartConstruction(int circularFactoryId, Dependency forDependency) + throws InternalProvisionException { + @SuppressWarnings("unchecked") + T t = (T) insert(circularFactoryId, forDependency, null); + return t; + } + + @Override + void finishConstruction(int circularFactoryId, @Nullable Object result) { + Object context = remove(circularFactoryId); + setProxyDelegates(context, result); + } + + @Override + void finishConstructionAndSetReference(int circularFactoryId, Object result) { + int index = get(circularFactoryId); + Object[] constructionContexts = this.constructionContexts; + Object context = constructionContexts[index]; + setProxyDelegates(context, result); + constructionContexts[index] = result; + } + + @Override + void clearCurrentReference(int circularFactoryId) { + remove(circularFactoryId); + } + + private void setProxyDelegates(Object context, Object result) { + if (context instanceof ProxyDelegates) { + for (DelegatingInvocationHandler handler : (ProxyDelegates) context) { + handler.setDelegate(result); + } + } + } + + /** + * Returns the index of where the given key should be inserted, or the index where it already is + * if present. To tell the difference, check the value in the construction contexts array. + */ + @Nullable + Object insert(int key, @Nullable Dependency forDependency, @Nullable Object existing) + throws InternalProvisionException { + int[] table = this.table; + int len = table.length; + int index = hash(key, len); + int c = table[index]; + int distance = 0; + while (true) { + if (c == 0) { + table[index] = key; + this.constructionContexts[index] = existing; + int newSize = ++this.tableSize; + if (newSize + (newSize >> 1) > len) { + expandTable(); + } + return null; + } + if (c == key) { + Object current = this.constructionContexts[index]; + // This must be a value set by setCurrentReference, just return it. + if (current != null && !(current instanceof ProxyDelegates)) { + return current; + } + // now we need to check if we can proxy the class + Class raw = forDependency.getKey().getTypeLiteral().getRawType(); + if (!raw.isInterface()) { + throw InternalProvisionException.cannotProxyClass(raw); + } + if (current == null) { + current = new ProxyDelegates(); + this.constructionContexts[index] = current; + } + DelegatingInvocationHandler handler = new DelegatingInvocationHandler<>(); + ((ProxyDelegates) current).add(handler); + return BytecodeGen.newCircularProxy(raw, handler); + } + // See how far is the current key from its ideal slot in the table. + int cDistance = distance(c, index, len); + if (cDistance < distance) { + // Steal from the rich! + table[index] = key; + Object[] constructionContexts = this.constructionContexts; + Object prev = constructionContexts[index]; + constructionContexts[index] = existing; + // Re-insert the key since we have stolen its slot. + insert(c, null, prev); + return null; + } + // Move to the next slot. + index = nextIndex(index, len); + distance++; + c = table[index]; + } + } + + /** Returns the index of where the given key is stored, or -1 if not found. */ + @VisibleForTesting + int get(int key) { + int[] table = this.table; + int len = table.length; + int index = hash(key, len); + int c = table[index]; + int distance = 0; + while (true) { + if (c == 0) { + return -1; + } + if (c == key) { + return index; + } + int cDistance = distance(c, index, len); + if (cDistance < distance) { + // This means that we have transitioned into another bucket chain. + return -1; + } + index = nextIndex(index, len); + distance++; + c = table[index]; + } + } + + Object remove(int key) { + int[] table = this.table; + int len = table.length; + int index = hash(key, len); + int c = table[index]; + int distance = 0; + while (true) { + if (c == 0) { + throw new IllegalStateException("table corrupted, key not found"); + } + if (c == key) { + table[index] = 0; + // NOTE: We don't currently resize when removing items. + this.tableSize--; + Object[] constructionContexts = this.constructionContexts; + Object context = constructionContexts[index]; + constructionContexts[index] = null; + // At this point we have potentially violated the robin hood invariant so we need to shift + // following items back. + // We shift each following item back by 1 unless it is empty or it is has a distance of + // zero. + int nextIndex = nextIndex(index, len); + int nextC = table[nextIndex]; + while (true) { + if (nextC == 0) { + break; // empty slot + } + if (hash(nextC, len) == nextIndex) { + break; // this item is in its ideal slot, so we cannot shift it + } + table[index] = nextC; + constructionContexts[index] = constructionContexts[nextIndex]; + table[nextIndex] = 0; + constructionContexts[nextIndex] = null; + index = nextIndex; + nextIndex = nextIndex(nextIndex, len); + nextC = table[nextIndex]; + } + return context; + } + int cDistance = distance(c, index, len); + if (cDistance < distance) { + // This means that we have transitioned into another bucket chain. + throw new IllegalStateException("table corrupted, key not found"); + } + index = nextIndex(index, len); + distance++; + c = table[index]; + } + } + + private void expandTable() throws InternalProvisionException { + int[] oldTable = this.table; + int oldLength = oldTable.length; + int newLength = oldLength * 2; + if (oldLength >= newLength) { // overflow + throw new IllegalArgumentException("Table size too large to expand: " + oldLength); + } + Object[] oldConstructionContexts = this.constructionContexts; + this.table = new int[newLength]; + this.constructionContexts = new Object[newLength]; + this.tableSize = 0; + for (int j = 0; j < oldLength; j += 1) { + int key = oldTable[j]; + if (key != 0) { + insert(key, null, oldConstructionContexts[j]); + } + } + } + } + + /** + * Returns the probe sequence length of the given key assuming it was found in the table at + * `index`. + */ + private static int distance(int key, int index, int len) { + int idealIndex = hash(key, len); + int naiveDistance = index - idealIndex; + if (naiveDistance < 0) { + // Handle the case where we have wrapped around the table. + return naiveDistance + len; + } + return naiveDistance; + } + + private static int nextIndex(int index, int len) { + // This is a circular buffer, so we need to wrap around if we are at the end. + // This pattern is used instead of a mask since after inlining it makes it easier for the VM to + // eliminate bounds checks on the array. + return index + 1 < len ? index + 1 : 0; + } + + private static int hash(int key, int len) { + return key & (len - 1); + } } diff --git a/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java b/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java index 3c22e6c8f5..efb22d9379 100644 --- a/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java +++ b/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java @@ -35,8 +35,9 @@ final class InternalFactoryToInitializableAdapter extends ProviderInternalFac public InternalFactoryToInitializableAdapter( Initializable> initializable, Object source, - ProvisionListenerStackCallback provisionCallback) { - super(source); + ProvisionListenerStackCallback provisionCallback, + int circularFactoryId) { + super(source, circularFactoryId); this.provisionCallback = provisionCallback; this.initializable = checkNotNull(initializable, "provider"); } @@ -47,18 +48,6 @@ public T get(InternalContext context, Dependency dependency, boolean linked) return circularGet(initializable.get(), context, dependency, provisionCallback); } - @Override - protected T provision( - jakarta.inject.Provider provider, - Dependency dependency, - ConstructionContext constructionContext) - throws InternalProvisionException { - try { - return super.provision(provider, dependency, constructionContext); - } catch (RuntimeException userException) { - throw InternalProvisionException.errorInProvider(userException).addSource(source); - } - } @Override public String toString() { diff --git a/core/src/com/google/inject/internal/InternalProviderInstanceBindingImpl.java b/core/src/com/google/inject/internal/InternalProviderInstanceBindingImpl.java index 5ff53952ea..2a9accfc3c 100644 --- a/core/src/com/google/inject/internal/InternalProviderInstanceBindingImpl.java +++ b/core/src/com/google/inject/internal/InternalProviderInstanceBindingImpl.java @@ -7,7 +7,6 @@ import com.google.inject.spi.Dependency; import com.google.inject.spi.HasDependencies; import com.google.inject.spi.InjectionPoint; -import com.google.inject.spi.ProviderWithExtensionVisitor; /** * A {@link ProviderInstanceBindingImpl} for implementing 'native' guice extensions. @@ -64,10 +63,7 @@ public void initialize(final InjectorImpl injector, final Errors errors) throws originalFactory.initialize(injector, errors); } - /** - * A base factory implementation. Any Factories that delegate to other bindings should use the - * {@code CyclicFactory} subclass, but trivial factories can use this one. - */ + /** A base factory implementation. */ abstract static class Factory implements InternalFactory, Provider, HasDependencies { private final InitializationTiming initializationTiming; private Object source; @@ -131,65 +127,4 @@ public T call() throws InternalProvisionException { protected abstract T doProvision(InternalContext context, Dependency dependency) throws InternalProvisionException; } - - /** - * An base factory implementation that can be extended to provide a specialized implementation of - * a {@link ProviderWithExtensionVisitor} and also implements {@link InternalFactory} - */ - abstract static class CyclicFactory extends Factory { - - CyclicFactory(InitializationTiming initializationTiming) { - super(initializationTiming); - } - - @Override - public final T get( - final InternalContext context, final Dependency dependency, boolean linked) - throws InternalProvisionException { - final ConstructionContext constructionContext = context.getConstructionContext(this); - // We have a circular reference between bindings. Return a proxy. - if (constructionContext.isConstructing()) { - Class expectedType = dependency.getKey().getTypeLiteral().getRawType(); - @SuppressWarnings("unchecked") - T proxyType = - (T) constructionContext.createProxy(context.getInjectorOptions(), expectedType); - return proxyType; - } - // Optimization: Don't go through the callback stack if no one's listening. - constructionContext.startConstruction(); - try { - if (provisionCallback == null) { - return provision(dependency, context, constructionContext); - } else { - return provisionCallback.provision( - context, - new ProvisionCallback() { - @Override - public T call() throws InternalProvisionException { - return provision(dependency, context, constructionContext); - } - }); - } - } finally { - constructionContext.removeCurrentReference(); - constructionContext.finishConstruction(); - } - } - - private T provision( - Dependency dependency, - InternalContext context, - ConstructionContext constructionContext) - throws InternalProvisionException { - try { - T t = doProvision(context, dependency); - constructionContext.setProxyDelegates(t); - return t; - } catch (InternalProvisionException ipe) { - throw ipe.addSource(getSource()); - } catch (Throwable t) { - throw InternalProvisionException.errorInProvider(t).addSource(getSource()); - } - } - } } diff --git a/core/src/com/google/inject/internal/InternalProvisionException.java b/core/src/com/google/inject/internal/InternalProvisionException.java index 4209a0261d..8771893eb9 100644 --- a/core/src/com/google/inject/internal/InternalProvisionException.java +++ b/core/src/com/google/inject/internal/InternalProvisionException.java @@ -246,7 +246,7 @@ ImmutableList getErrors() { return builder.build(); } - /** Returns this exception convered to a ProvisionException. */ + /** Returns this exception converted to a ProvisionException. */ public ProvisionException toProvisionException() { ProvisionException exception = new ProvisionException(getErrors()); return exception; diff --git a/core/src/com/google/inject/internal/MembersInjectorImpl.java b/core/src/com/google/inject/internal/MembersInjectorImpl.java index 57a62c74b7..a2ba6119c8 100644 --- a/core/src/com/google/inject/internal/MembersInjectorImpl.java +++ b/core/src/com/google/inject/internal/MembersInjectorImpl.java @@ -166,6 +166,14 @@ void injectMembers(T t, InternalContext context, boolean toolableOnly) } } + /** + * Returns true if a call to {@link #injectMembers} and {@link #notifyListeners} would have no + * effect. + */ + boolean isEmpty() { + return memberInjectors == null && userMembersInjectors == null && injectionListeners == null; + } + @Override public String toString() { return "MembersInjector<" + typeLiteral + ">"; diff --git a/core/src/com/google/inject/internal/ProvidedByInternalFactory.java b/core/src/com/google/inject/internal/ProvidedByInternalFactory.java index 3cc08f3f26..008a7d8cec 100644 --- a/core/src/com/google/inject/internal/ProvidedByInternalFactory.java +++ b/core/src/com/google/inject/internal/ProvidedByInternalFactory.java @@ -32,14 +32,15 @@ class ProvidedByInternalFactory extends ProviderInternalFactory implements private final Class rawType; private final Class> providerType; private final Key> providerKey; - private BindingImpl> providerBinding; + private InternalFactory> providerFactory; private ProvisionListenerStackCallback provisionCallback; ProvidedByInternalFactory( Class rawType, Class> providerType, - Key> providerKey) { - super(providerKey); + Key> providerKey, + int circularFactoryId) { + super(providerKey, circularFactoryId); this.rawType = rawType; this.providerType = providerType; this.providerKey = providerKey; @@ -51,43 +52,37 @@ void setProvisionListenerCallback(ProvisionListenerStackCallback listener) { @Override public void initialize(InjectorImpl injector, Errors errors) throws ErrorsException { - providerBinding = - injector.getBindingOrThrow(providerKey, errors, JitLimitation.NEW_OR_EXISTING_JIT); + providerFactory = + injector.getInternalFactory(providerKey, errors, JitLimitation.NEW_OR_EXISTING_JIT); } @Override public T get(InternalContext context, Dependency dependency, boolean linked) throws InternalProvisionException { - BindingImpl> localProviderBinding = providerBinding; - if (localProviderBinding == null) { + InternalFactory> localProviderFactory = providerFactory; + if (localProviderFactory == null) { throw new IllegalStateException("not initialized"); } - Key> localProviderKey = providerKey; try { - Provider provider = - localProviderBinding.getInternalFactory().get(context, dependency, true); + Provider provider = localProviderFactory.get(context, dependency, true); return circularGet(provider, context, dependency, provisionCallback); } catch (InternalProvisionException ipe) { - throw ipe.addSource(localProviderKey); + throw ipe.addSource(providerKey); } } @Override protected T provision( jakarta.inject.Provider provider, - Dependency dependency, - ConstructionContext constructionContext) + InternalContext context, + Dependency dependency) throws InternalProvisionException { - try { - Object o = super.provision(provider, dependency, constructionContext); - if (o != null && !rawType.isInstance(o)) { - throw InternalProvisionException.subtypeNotProvided(providerType, rawType); - } - @SuppressWarnings("unchecked") // protected by isInstance() check above - T t = (T) o; - return t; - } catch (RuntimeException e) { - throw InternalProvisionException.errorInProvider(e).addSource(source); + Object o = super.provision(provider, context, dependency); + if (o != null && !rawType.isInstance(o)) { + throw InternalProvisionException.subtypeNotProvided(providerType, rawType).addSource(source); } + @SuppressWarnings("unchecked") // protected by isInstance() check above + T t = (T) o; + return t; } } diff --git a/core/src/com/google/inject/internal/ProviderInternalFactory.java b/core/src/com/google/inject/internal/ProviderInternalFactory.java index 54bdf811d0..14e38e60fd 100644 --- a/core/src/com/google/inject/internal/ProviderInternalFactory.java +++ b/core/src/com/google/inject/internal/ProviderInternalFactory.java @@ -31,9 +31,11 @@ abstract class ProviderInternalFactory implements InternalFactory { protected final Object source; + private final int circularFactoryId; - ProviderInternalFactory(Object source) { + ProviderInternalFactory(Object source, int circularFactoryId) { this.source = checkNotNull(source, "source"); + this.circularFactoryId = circularFactoryId; } protected T circularGet( @@ -42,36 +44,25 @@ protected T circularGet( final Dependency dependency, @Nullable ProvisionListenerStackCallback provisionCallback) throws InternalProvisionException { - final ConstructionContext constructionContext = context.getConstructionContext(this); - - // We have a circular reference between constructors. Return a proxy. - if (constructionContext.isConstructing()) { - Class expectedType = dependency.getKey().getTypeLiteral().getRawType(); - // TODO: if we can't proxy this object, can we proxy the other object? - @SuppressWarnings("unchecked") - T proxyType = (T) constructionContext.createProxy(context.getInjectorOptions(), expectedType); - return proxyType; + @SuppressWarnings("unchecked") + T result = (T) context.tryStartConstruction(circularFactoryId, dependency); + if (result != null) { + // We have a circular reference between bindings. Return a proxy. + return result; } - // Optimization: Don't go through the callback stack if no one's listening. - constructionContext.startConstruction(); - try { - if (provisionCallback == null) { - return provision(provider, dependency, constructionContext); - } else { - return provisionCallback.provision( - context, - new ProvisionCallback() { - @Override - public T call() throws InternalProvisionException { - return provision(provider, dependency, constructionContext); - } - }); + if (provisionCallback == null) { + return provision(provider, context, dependency); + } else { + return provisionCallback.provision( + context, + new ProvisionCallback() { + @Override + public T call() throws InternalProvisionException { + return provision(provider, context, dependency); + } + }); } - } finally { - constructionContext.removeCurrentReference(); - constructionContext.finishConstruction(); - } } /** @@ -79,15 +70,19 @@ public T call() throws InternalProvisionException { * ErrorsExceptions. */ protected T provision( - Provider provider, - Dependency dependency, - ConstructionContext constructionContext) + Provider provider, InternalContext context, Dependency dependency) throws InternalProvisionException { - T t = provider.get(); + T t = null; + try { + t = provider.get(); + } catch (RuntimeException e) { + throw InternalProvisionException.errorInProvider(e).addSource(source); + } finally { + context.finishConstruction(circularFactoryId, t); + } if (t == null && !dependency.isNullable()) { InternalProvisionException.onNullInjectedIntoNonNullableDependency(source, dependency); } - constructionContext.setProxyDelegates(t); return t; } } diff --git a/core/src/com/google/inject/internal/ProviderMethod.java b/core/src/com/google/inject/internal/ProviderMethod.java index c3ad453887..db9641433a 100644 --- a/core/src/com/google/inject/internal/ProviderMethod.java +++ b/core/src/com/google/inject/internal/ProviderMethod.java @@ -24,6 +24,7 @@ import com.google.inject.PrivateBinder; import com.google.inject.Provides; import com.google.inject.internal.InternalProviderInstanceBindingImpl.InitializationTiming; +import com.google.inject.internal.ProvisionListenerStackCallback.ProvisionCallback; import com.google.inject.internal.util.StackTraceElements; import com.google.inject.spi.BindingTargetVisitor; import com.google.inject.spi.Dependency; @@ -44,7 +45,7 @@ * * @author jessewilson@google.com (Jesse Wilson) */ -public abstract class ProviderMethod extends InternalProviderInstanceBindingImpl.CyclicFactory +public abstract class ProviderMethod extends InternalProviderInstanceBindingImpl.Factory implements HasDependencies, ProvidesMethodBinding, ProviderWithExtensionVisitor { /** @@ -92,13 +93,16 @@ static ProviderMethod create( private final ImmutableSet> dependencies; private final boolean exposed; private final Annotation annotation; + private int circularFactoryId; /** * Set by {@link #initialize(InjectorImpl, Errors)} so it is always available prior to injection. */ private SingleParameterInjector[] parameterInjectors; - /** @param method the method to invoke. Its return type must be the same type as {@code key}. */ + /** + * @param method the method to invoke. Its return type must be the same type as {@code key}. + */ ProviderMethod( Key key, Method method, @@ -162,25 +166,62 @@ public void configure(Binder binder) { @Override void initialize(InjectorImpl injector, Errors errors) throws ErrorsException { parameterInjectors = injector.getParametersInjectors(dependencies.asList(), errors); + circularFactoryId = injector.circularFactoryIdFactory.next(); } @Override - protected T doProvision(InternalContext context, Dependency dependency) + public final T get(final InternalContext context, final Dependency dependency, boolean linked) throws InternalProvisionException { + @SuppressWarnings("unchecked") + T result = (T) context.tryStartConstruction(circularFactoryId, dependency); + if (result != null) { + // We have a circular reference between bindings. Return a proxy. + return result; + } + // Optimization: Don't go through the callback stack if no one's listening. + if (provisionCallback == null) { + return provision(dependency, context); + } else { + return provisionCallback.provision( + context, + new ProvisionCallback() { + @Override + public T call() throws InternalProvisionException { + return provision(dependency, context); + } + }); + } + } + + private T provision(Dependency dependency, InternalContext context) + throws InternalProvisionException { + T t = null; try { - T t = doProvision(SingleParameterInjector.getAll(context, parameterInjectors)); + t = doProvision(SingleParameterInjector.getAll(context, parameterInjectors)); if (t == null && !dependency.isNullable()) { InternalProvisionException.onNullInjectedIntoNonNullableDependency(getMethod(), dependency); } return t; } catch (IllegalAccessException e) { throw new AssertionError(e); + } catch (InternalProvisionException e) { + throw e.addSource(getSource()); } catch (InvocationTargetException userException) { Throwable cause = userException.getCause() != null ? userException.getCause() : userException; throw InternalProvisionException.errorInProvider(cause).addSource(getSource()); + } catch (Throwable unexpected) { + throw InternalProvisionException.errorInProvider(unexpected).addSource(getSource()); + } finally { + context.finishConstruction(circularFactoryId, t); } } + @Override + protected T doProvision(InternalContext context, Dependency dependency) + throws InternalProvisionException { + throw new AssertionError(); // should never be called since we override get() + } + /** Extension point for our subclasses to implement the provisioning strategy. */ abstract T doProvision(Object[] parameters) throws IllegalAccessException, InvocationTargetException; diff --git a/core/src/com/google/inject/internal/SingletonScope.java b/core/src/com/google/inject/internal/SingletonScope.java index ffb2a87fbc..40f25b4b8c 100644 --- a/core/src/com/google/inject/internal/SingletonScope.java +++ b/core/src/com/google/inject/internal/SingletonScope.java @@ -14,6 +14,7 @@ import com.google.inject.internal.CycleDetectingLock.CycleDetectingLockFactory; import com.google.inject.spi.Dependency; import com.google.inject.spi.Message; +import java.util.ArrayList; import java.util.Formatter; import java.util.List; import javax.annotation.Nullable; @@ -107,12 +108,19 @@ public Provider scope(final Key key, final Provider creator) { volatile Object instance; /** - * Circular proxies are used when potential deadlocks are detected. Guarded by itself. - * ConstructionContext is not thread-safe, so each call should be synchronized. + * Circular proxies are used when potential deadlocks are detected. This lock is used to guard + * accesses to the list of invocation handlers. * *

Locking strategy: manipulations with proxies list or instance initialization. */ - final ConstructionContext constructionContext = new ConstructionContext<>(); + final Object proxyCycleLock = new Object(); + + /** + * List of invocation handlers for circular proxies. Typically this is `null` as we don't + * allocate proxies, but when we do, we allocate a new list and store the handlers here until + * the instance is initialized. + */ + List> invocationHandlers; /** * For each binding there is a separate lock that we hold during object creation. @@ -179,10 +187,16 @@ public T get() { return provided; } - synchronized (constructionContext) { + synchronized (proxyCycleLock) { // guarantee thread-safety for instance and proxies initialization instance = providedNotNull; - constructionContext.setProxyDelegates(provided); + if (invocationHandlers != null) { + for (DelegatingInvocationHandler handler : invocationHandlers) { + handler.setDelegate(provided); + } + // initialization of each handler can happen no more than once + invocationHandlers = null; + } } } else { // safety assert in case instance was initialized @@ -192,10 +206,11 @@ public T get() { } } } catch (RuntimeException e) { - // something went wrong, be sure to clean a construction context - // this helps to prevent potential memory leaks in circular proxies list - synchronized (constructionContext) { - constructionContext.finishConstruction(); + // something went wrong, be sure to clean up our proxy list + synchronized (proxyCycleLock) { + // TODO it might be better to set the delegate to something that throws this + // exception, instead + invocationHandlers = null; } throw e; } finally { @@ -208,7 +223,7 @@ public T get() { ImmutableList.of(createCycleDependenciesMessage(locksCycle, null))); } // potential deadlock detected, creation lock is not taken by this thread - synchronized (constructionContext) { + synchronized (proxyCycleLock) { // guarantee thread-safety for instance and proxies initialization if (instance == null) { // creating a proxy to satisfy circular dependency across several threads @@ -218,9 +233,22 @@ public T get() { Class rawType = dependency.getKey().getTypeLiteral().getRawType(); try { + if (!context.areCircularProxiesEnabled()) { + throw InternalProvisionException.circularDependenciesDisabled(rawType); + } + if (!rawType.isInterface()) { + throw InternalProvisionException.cannotProxyClass(rawType); + } + if (invocationHandlers == null) { + invocationHandlers = new ArrayList<>(); + } + + DelegatingInvocationHandler invocationHandler = + new DelegatingInvocationHandler<>(); + invocationHandlers.add(invocationHandler); + @SuppressWarnings("unchecked") - T proxy = - (T) constructionContext.createProxy(context.getInjectorOptions(), rawType); + T proxy = (T) BytecodeGen.newCircularProxy(rawType, invocationHandler); return proxy; } catch (InternalProvisionException e) { // best effort to create a rich error message diff --git a/core/src/com/google/inject/internal/TypeConverterBindingProcessor.java b/core/src/com/google/inject/internal/TypeConverterBindingProcessor.java index b0b34a343e..24c51246ac 100644 --- a/core/src/com/google/inject/internal/TypeConverterBindingProcessor.java +++ b/core/src/com/google/inject/internal/TypeConverterBindingProcessor.java @@ -73,7 +73,7 @@ public String toString() { injector, Matchers.subclassesOf(Enum.class), new TypeConverter() { - @SuppressWarnings("rawtypes") // Unavoidable, only way to use Enum.valueOf + @SuppressWarnings({"rawtypes", "unchecked"}) // Unavoidable, only way to use Enum.valueOf @Override public Object convert(String value, TypeLiteral toType) { return Enum.valueOf((Class) toType.getRawType(), value); diff --git a/core/src/com/google/inject/matcher/Matchers.java b/core/src/com/google/inject/matcher/Matchers.java index 2a94cf98e3..79b60abc12 100644 --- a/core/src/com/google/inject/matcher/Matchers.java +++ b/core/src/com/google/inject/matcher/Matchers.java @@ -31,6 +31,7 @@ * * @author crazybob@google.com (Bob Lee) */ +@SuppressWarnings("rawtypes") // lots of preexisting issues, and it's not worth fixing them all public class Matchers { private Matchers() {} diff --git a/core/test/com/google/inject/BinderTest.java b/core/test/com/google/inject/BinderTest.java index be8008ccfd..76b85371f9 100644 --- a/core/test/com/google/inject/BinderTest.java +++ b/core/test/com/google/inject/BinderTest.java @@ -404,7 +404,6 @@ protected void configure() { }); fail(); } catch (CreationException expected) { - expected.printStackTrace(); assertContains( expected.getMessage(), "BinderTest$HasImplementedBy1 was bound multiple times.", diff --git a/core/test/com/google/inject/internal/InternalContextTest.java b/core/test/com/google/inject/internal/InternalContextTest.java new file mode 100644 index 0000000000..f58ffcdc52 --- /dev/null +++ b/core/test/com/google/inject/internal/InternalContextTest.java @@ -0,0 +1,215 @@ +/* + * Copyright (C) 2024 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.internal; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.inject.Key; +import com.google.inject.spi.Dependency; +import java.util.Random; +import java.util.stream.IntStream; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class InternalContextTest { + private static final Dependency DEP = Dependency.get(Key.get(String.class)); + private static final Dependency PROXYABLE_DEP = Dependency.get(Key.get(Runnable.class)); + + @Test + public void testWithoutProxySupport_throwsBecauseWeCannotProxy() + throws InternalProvisionException { + InternalContext.WithoutProxySupport context = + new InternalContext.WithoutProxySupport(new Object[1]); + context.tryStartConstruction(1, PROXYABLE_DEP); + InternalProvisionException ipe = + assertThrows( + InternalProvisionException.class, () -> context.tryStartConstruction(1, PROXYABLE_DEP)); + assertThat(ipe.toProvisionException()) + .hasMessageThat() + .contains("circular dependencies are disabled"); + } + + // Force a hash collision in the internal context table. + @Test + public void testWithoutProxySupport_forceHashCollision() throws InternalProvisionException { + // Initial table size is 16, so we should be able to to force hash collisions using keys that + // are === mod 16 + InternalContext.WithoutProxySupport context = + new InternalContext.WithoutProxySupport(new Object[1]); + context.insert(1, DEP); + assertThat(context.get(1)).isEqualTo(1); + context.insert(17, DEP); + assertThat(context.get(17)).isEqualTo(2); + context.insert(2, DEP); + assertThat(context.get(2)).isEqualTo(3); + context.insert(18, DEP); + assertThat(context.get(18)).isEqualTo(4); + context.remove(1); // This should trigger a shift + assertThat(context.get(1)).isEqualTo(-1); + assertThat(context.get(17)).isEqualTo(1); + assertThat(context.get(2)).isEqualTo(2); + assertThat(context.get(18)).isEqualTo(3); + context.remove(2); // This should trigger a shift + assertThat(context.get(1)).isEqualTo(-1); + assertThat(context.get(17)).isEqualTo(1); + assertThat(context.get(2)).isEqualTo(-1); + assertThat(context.get(18)).isEqualTo(2); + } + + @Test + public void testWithProxySupport_throwsWhenWeCannotProxy() throws InternalProvisionException { + InternalContext.WithProxySupport context = new InternalContext.WithProxySupport(new Object[1]); + context.tryStartConstruction(1, DEP); + // cannot proxy a String + InternalProvisionException ipe = + assertThrows(InternalProvisionException.class, () -> context.tryStartConstruction(1, DEP)); + assertThat(ipe.toProvisionException()).hasMessageThat().contains("but it is not an interface"); + } + + @Test + public void testWithProxySupport_returnsProxyWhenWeCan() throws InternalProvisionException { + InternalContext.WithProxySupport context = new InternalContext.WithProxySupport(new Object[1]); + assertThat(context.tryStartConstruction(1, PROXYABLE_DEP)).isNull(); + Runnable proxy = context.tryStartConstruction(1, PROXYABLE_DEP); + Runnable proxy2 = context.tryStartConstruction(1, PROXYABLE_DEP); + assertThat(BytecodeGen.isCircularProxy(proxy)).isTrue(); + int[] called = new int[1]; + context.finishConstruction(1, (Runnable) () -> called[0]++); + proxy.run(); // The proxy should call the real thing + assertThat(called[0]).isEqualTo(1); + proxy2.run(); // The proxy should call the real thing + assertThat(called[0]).isEqualTo(2); + } + + @Test + public void testWithProxySupport_multipleCollidingProxiesAcrossAResize() + throws InternalProvisionException { + InternalContext.WithProxySupport context = new InternalContext.WithProxySupport(new Object[1]); + // Initial table size is 16, so we should be able to to force hash collisions using keys that + // are === mod 16 + assertThat(context.tryStartConstruction(1, PROXYABLE_DEP)).isNull(); + assertThat(context.get(1)).isEqualTo(1); + assertThat(context.tryStartConstruction(17, PROXYABLE_DEP)).isNull(); + assertThat(context.get(17)).isEqualTo(2); + // this will also collide with 1 after a resize + assertThat(context.tryStartConstruction(33, PROXYABLE_DEP)).isNull(); + assertThat(context.get(33)).isEqualTo(3); + Runnable proxy1 = context.tryStartConstruction(1, PROXYABLE_DEP); + Runnable proxy2 = context.tryStartConstruction(17, PROXYABLE_DEP); + Runnable proxy3 = context.tryStartConstruction(33, PROXYABLE_DEP); + assertThat(BytecodeGen.isCircularProxy(proxy1)).isTrue(); + assertThat(BytecodeGen.isCircularProxy(proxy2)).isTrue(); + assertThat(BytecodeGen.isCircularProxy(proxy3)).isTrue(); + // Now force the table to resize. It resizes to 32 after it is 2/3 full. + for (int i = 2; i < 12; i++) { + assertThat(context.tryStartConstruction(i, DEP)).isNull(); + } + // Check where our original keys moved + assertThat(context.get(1)).isEqualTo(1); + assertThat(context.get(17)).isEqualTo(17); // no longer a collision + assertThat(context.get(33)).isEqualTo(2); // still a collision with 1 + + // Now complete the construction of the proxies and check that they call the right things. + int[] called = new int[3]; + context.finishConstruction(1, (Runnable) () -> called[0] = 1); + context.finishConstruction(17, (Runnable) () -> called[1] = 17); + context.finishConstruction(33, (Runnable) () -> called[2] = 33); + proxy1.run(); + assertThat(called).asList().containsExactly(1, 0, 0).inOrder(); + proxy2.run(); + assertThat(called).asList().containsExactly(1, 17, 0).inOrder(); + proxy3.run(); + assertThat(called).asList().containsExactly(1, 17, 33).inOrder(); + } + + // Force a hash collision in the internal context table. + @Test + public void testWithProxySupport_forceHashCollision() throws InternalProvisionException { + // Initial table size is 16, so we should be able to to force hash collisions using keys that + // are === mod 16 + InternalContext.WithProxySupport context = new InternalContext.WithProxySupport(new Object[1]); + context.insert(1, DEP, null); + assertThat(context.get(1)).isEqualTo(1); + context.insert(17, DEP, null); + assertThat(context.get(17)).isEqualTo(2); + context.insert(2, DEP, null); + assertThat(context.get(2)).isEqualTo(3); + context.insert(18, DEP, null); + assertThat(context.get(18)).isEqualTo(4); + context.remove(1); // This should trigger a shift + assertThat(context.get(1)).isEqualTo(-1); + assertThat(context.get(17)).isEqualTo(1); + assertThat(context.get(2)).isEqualTo(2); + assertThat(context.get(18)).isEqualTo(3); + context.remove(2); // This should trigger a shift + assertThat(context.get(1)).isEqualTo(-1); + assertThat(context.get(17)).isEqualTo(1); + assertThat(context.get(2)).isEqualTo(-1); + assertThat(context.get(18)).isEqualTo(2); + } + + /** + * Insert a bunch of keys and check that our 2 table implementations are storing them in the same + * place. + * + *

While the code is somewhat duplicated we can at least ensure that the two implementations + * are consistent. + */ + @Test + public void testFuzzTables() throws InternalProvisionException { + int[] keys = IntStream.range(1, 1024 * 1024).toArray(); + + Random rnd = new Random(12345); + for (int i = 0; i < 30; i++) { + InternalContext.WithoutProxySupport withoutProxySupport = + new InternalContext.WithoutProxySupport(new Object[1]); + InternalContext.WithProxySupport withProxySupport = + new InternalContext.WithProxySupport(new Object[1]); + for (int key : keys) { + // insert throws on duplicates... so no need to check return values + withoutProxySupport.insert(key, DEP); + withProxySupport.insert(key, DEP, null); + // They should have stored them in the same place. + assertThat(withoutProxySupport.get(key)).isEqualTo(withProxySupport.get(key)); + } + + // Remove in a different order than how we inserted them. + for (int key : shuffleArray(keys.clone(), rnd)) { + // remove throws if we cannot find the key... so no need to check return values + withoutProxySupport.remove(key); + withProxySupport.remove(key); + } + // shuffle the keys so we insert in a different order next time, this ensures that we will + // trigger some collisions + shuffleArray(keys, rnd); + } + } + + @CanIgnoreReturnValue + private static int[] shuffleArray(int[] arr, Random rnd) { + for (int i = arr.length - 1; i > 0; i--) { + int index = rnd.nextInt(i + 1); + // Simple swap + int a = arr[index]; + arr[index] = arr[i]; + arr[i] = a; + } + return arr; + } +} diff --git a/core/test/com/googlecode/guice/BytecodeGenTest.java b/core/test/com/googlecode/guice/BytecodeGenTest.java index 6fe34ab779..723c6d43df 100644 --- a/core/test/com/googlecode/guice/BytecodeGenTest.java +++ b/core/test/com/googlecode/guice/BytecodeGenTest.java @@ -187,6 +187,7 @@ protected Class loadClass(final String name, final boolean resolve) private Module testModule; @Before + @SuppressWarnings("unchecked") public void setUp() throws Exception { assumeTrue(InternalFlags.isBytecodeGenEnabled());