From 31b1e22561d5db89abfd09a8ecd9932d20f5eda3 Mon Sep 17 00:00:00 2001 From: Mikolaj Izdebski Date: Mon, 18 May 2015 12:05:45 +0200 Subject: [PATCH] Revert "Some work on issue 910 -- ensure that anonymous keys & typeliterals don't" This reverts commit 825f8c1df885b9d7643a9e18e336984f0138edaf. --- .../com/google/inject/internal/InjectorImpl.java | 1 - core/src/com/google/inject/internal/MoreTypes.java | 37 ++----------------- core/src/com/google/inject/spi/Dependency.java | 3 +- core/src/com/google/inject/spi/Elements.java | 14 +++---- core/test/com/google/inject/Asserts.java | 42 --------------------- core/test/com/google/inject/KeyTest.java | 43 ---------------------- .../com/google/inject/internal/WeakKeySetTest.java | 11 +++++- .../google/inject/internal/WeakKeySetUtils.java | 42 +++++++++++++++++++++ .../google/inject/multibindings/MapBinderTest.java | 3 +- .../inject/multibindings/OptionalBinderTest.java | 3 +- 10 files changed, 62 insertions(+), 137 deletions(-) diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java index 54ce8a3..d260046 100644 --- a/core/src/com/google/inject/internal/InjectorImpl.java +++ b/core/src/com/google/inject/internal/InjectorImpl.java @@ -801,7 +801,6 @@ final class InjectorImpl implements Injector, Lookups { 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); diff --git a/core/src/com/google/inject/internal/MoreTypes.java b/core/src/com/google/inject/internal/MoreTypes.java index bdf6029..12a7625 100644 --- a/core/src/com/google/inject/internal/MoreTypes.java +++ b/core/src/com/google/inject/internal/MoreTypes.java @@ -23,7 +23,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.base.Objects; import com.google.common.collect.ImmutableMap; import com.google.inject.ConfigurationException; -import com.google.inject.Key; import com.google.inject.TypeLiteral; import com.google.inject.util.Types; @@ -65,25 +64,6 @@ public class MoreTypes { .build(); /** - * Returns a key that doesn't hold any references to parent classes. - * This is necessary for anonymous keys, so ensure we don't hold a ref - * to the containing module (or class) forever. - */ - public static Key canonicalizeKey(Key key) { - // If we know this isn't a subclass, return as-is. - // Otherwise, recreate the key to avoid the subclass - if (key.getClass() == Key.class) { - return key; - } else if (key.getAnnotation() != null) { - return Key.get(key.getTypeLiteral(), key.getAnnotation()); - } else if (key.getAnnotationType() != null) { - return Key.get(key.getTypeLiteral(), key.getAnnotationType()); - } else { - return Key.get(key.getTypeLiteral()); - } - } - - /** * Returns an type that's appropriate for use in a key. * *

If the raw type of {@code typeLiteral} is a {@code javax.inject.Provider}, this returns a @@ -113,20 +93,9 @@ public class MoreTypes { @SuppressWarnings("unchecked") TypeLiteral wrappedPrimitives = (TypeLiteral) PRIMITIVE_TO_WRAPPER.get(typeLiteral); - if (wrappedPrimitives != null) { - return wrappedPrimitives; - } - - // If we know this isn't a subclass, return as-is. - if (typeLiteral.getClass() == TypeLiteral.class) { - return typeLiteral; - } - - // recreate the TypeLiteral to avoid anonymous TypeLiterals from holding refs to their - // surrounding classes. - @SuppressWarnings("unchecked") - TypeLiteral recreated = (TypeLiteral) TypeLiteral.get(typeLiteral.getType()); - return recreated; + return wrappedPrimitives != null + ? wrappedPrimitives + : typeLiteral; } /** diff --git a/core/src/com/google/inject/spi/Dependency.java b/core/src/com/google/inject/spi/Dependency.java index f86e255..c51d87c 100644 --- a/core/src/com/google/inject/spi/Dependency.java +++ b/core/src/com/google/inject/spi/Dependency.java @@ -22,7 +22,6 @@ import com.google.common.base.Objects; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.inject.Key; -import com.google.inject.internal.MoreTypes; import java.util.List; import java.util.Set; @@ -55,7 +54,7 @@ public final class Dependency { * nullable. */ public static Dependency get(Key key) { - return new Dependency(null, MoreTypes.canonicalizeKey(key), true, -1); + return new Dependency(null, key, true, -1); } /** diff --git a/core/src/com/google/inject/spi/Elements.java b/core/src/com/google/inject/spi/Elements.java index 9348276..f5bbe89 100644 --- a/core/src/com/google/inject/spi/Elements.java +++ b/core/src/com/google/inject/spi/Elements.java @@ -44,7 +44,6 @@ import com.google.inject.internal.ConstantBindingBuilderImpl; import com.google.inject.internal.Errors; import com.google.inject.internal.ExposureBuilder; import com.google.inject.internal.InternalFlags.IncludeStackTraceOption; -import com.google.inject.internal.MoreTypes; import com.google.inject.internal.PrivateElementsImpl; import com.google.inject.internal.ProviderMethodsModule; import com.google.inject.internal.util.SourceProvider; @@ -243,14 +242,13 @@ public final class Elements { @Override public void requestInjection(TypeLiteral type, T instance) { - elements.add(new InjectionRequest(getElementSource(), MoreTypes.canonicalizeForKey(type), - instance)); + elements.add(new InjectionRequest(getElementSource(), type, instance)); } @Override public MembersInjector getMembersInjector(final TypeLiteral typeLiteral) { - final MembersInjectorLookup element = new MembersInjectorLookup(getElementSource(), - MoreTypes.canonicalizeForKey(typeLiteral)); + final MembersInjectorLookup element + = new MembersInjectorLookup(getElementSource(), typeLiteral); elements.add(element); return element.getMembersInjector(); } @@ -372,8 +370,7 @@ public final class Elements { } public AnnotatedBindingBuilder bind(Key key) { - BindingBuilder builder = - new BindingBuilder(this, elements, getElementSource(), MoreTypes.canonicalizeKey(key)); + BindingBuilder builder = new BindingBuilder(this, elements, getElementSource(), key); return builder; } @@ -483,8 +480,7 @@ public final class Elements { }; } - ExposureBuilder builder = - new ExposureBuilder(this, getElementSource(), MoreTypes.canonicalizeKey(key)); + ExposureBuilder builder = new ExposureBuilder(this, getElementSource(), key); privateElements.addExposureBuilder(builder); return builder; } diff --git a/core/test/com/google/inject/Asserts.java b/core/test/com/google/inject/Asserts.java index 6c63158..363e161 100644 --- a/core/test/com/google/inject/Asserts.java +++ b/core/test/com/google/inject/Asserts.java @@ -21,14 +21,12 @@ import static com.google.inject.internal.InternalFlags.IncludeStackTraceOption; import static com.google.inject.internal.InternalFlags.getIncludeStackTraceOption; import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertNotNull; -import static junit.framework.Assert.assertSame; import static junit.framework.Assert.assertTrue; import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.common.testing.GcFinalization; import junit.framework.Assert; @@ -38,8 +36,6 @@ import java.io.IOException; import java.io.NotSerializableException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; -import java.lang.ref.ReferenceQueue; -import java.lang.ref.WeakReference; /** * @author jessewilson@google.com (Jesse Wilson) @@ -163,42 +159,4 @@ public class Asserts { } catch (NotSerializableException expected) { } } - - public static void awaitFullGc() { - // GcFinalization *should* do it, but doesn't work well in practice... - // so we put a second latch and wait for a ReferenceQueue to tell us. - ReferenceQueue queue = new ReferenceQueue(); - WeakReference ref = new WeakReference(new Object(), queue); - GcFinalization.awaitFullGc(); - try { - assertSame("queue didn't return ref in time", ref, queue.remove(5000)); - } catch (IllegalArgumentException e) { - throw new RuntimeException(e); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - } - - public static void awaitClear(WeakReference ref) { - // GcFinalization *should* do it, but doesn't work well in practice... - // so we put a second latch and wait for a ReferenceQueue to tell us. - Object data = ref.get(); - ReferenceQueue queue = null; - WeakReference extraRef = null; - if (data != null) { - queue = new ReferenceQueue(); - extraRef = new WeakReference(data, queue); - data = null; - } - GcFinalization.awaitClear(ref); - if (queue != null) { - try { - assertSame("queue didn't return ref in time", extraRef, queue.remove(5000)); - } catch (IllegalArgumentException e) { - throw new RuntimeException(e); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - } - } } diff --git a/core/test/com/google/inject/KeyTest.java b/core/test/com/google/inject/KeyTest.java index d9dd943..c0401e0 100644 --- a/core/test/com/google/inject/KeyTest.java +++ b/core/test/com/google/inject/KeyTest.java @@ -19,12 +19,10 @@ package com.google.inject; import static com.google.inject.Asserts.assertContains; import static com.google.inject.Asserts.assertEqualsBothWays; import static com.google.inject.Asserts.assertNotSerializable; -import static com.google.inject.Asserts.awaitClear; import static java.lang.annotation.RetentionPolicy.RUNTIME; import com.google.inject.name.Named; import com.google.inject.name.Names; -import com.google.inject.spi.Dependency; import com.google.inject.util.Types; import junit.framework.TestCase; @@ -33,15 +31,12 @@ import java.io.IOException; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.Target; -import java.lang.ref.WeakReference; import java.lang.reflect.Method; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.lang.reflect.TypeVariable; -import java.util.ArrayList; import java.util.List; import java.util.Map; -import java.util.concurrent.atomic.AtomicReference; /** * @author crazybob@google.com (Bob Lee) @@ -280,42 +275,4 @@ public class KeyTest extends TestCase { @Marker class HasAnnotations {} - public void testAnonymousClassesDontHoldRefs() { - final AtomicReference>> stringProvider = - new AtomicReference>>(); - final AtomicReference>> intProvider = - new AtomicReference>>(); - final Object foo = new Object() { - @SuppressWarnings("unused") @Inject List list; - }; - Module module = new AbstractModule() { - @Override protected void configure() { - bind(new Key>() {}).toInstance(new ArrayList()); - bind(new TypeLiteral>() {}).toInstance(new ArrayList()); - - stringProvider.set(getProvider(new Key>() {})); - intProvider.set(binder().getProvider(Dependency.get(new Key>() {}))); - - binder().requestInjection(new TypeLiteral() {}, foo); - } - }; - WeakReference moduleRef = new WeakReference(module); - final Injector injector = Guice.createInjector(module); - module = null; - awaitClear(moduleRef); // Make sure anonymous keys & typeliterals don't hold the module. - - Runnable runner = new Runnable() { - @Override public void run() { - injector.getInstance(new Key>() {}); - injector.getInstance(Key.get(new TypeLiteral>() {})); - } - }; - WeakReference runnerRef = new WeakReference(runner); - runner.run(); - runner = null; - awaitClear(runnerRef); // also make sure anonymous keys & typeliterals don't hold for JITs - } - - static class Typed {} - } diff --git a/core/test/com/google/inject/internal/WeakKeySetTest.java b/core/test/com/google/inject/internal/WeakKeySetTest.java index 4a81ebb..3797d88 100644 --- a/core/test/com/google/inject/internal/WeakKeySetTest.java +++ b/core/test/com/google/inject/internal/WeakKeySetTest.java @@ -16,13 +16,13 @@ package com.google.inject.internal; -import static com.google.inject.Asserts.awaitClear; -import static com.google.inject.Asserts.awaitFullGc; import static com.google.inject.internal.WeakKeySetUtils.assertBlacklisted; import static com.google.inject.internal.WeakKeySetUtils.assertInSet; import static com.google.inject.internal.WeakKeySetUtils.assertNotBlacklisted; import static com.google.inject.internal.WeakKeySetUtils.assertNotInSet; import static com.google.inject.internal.WeakKeySetUtils.assertSourceNotInSet; +import static com.google.inject.internal.WeakKeySetUtils.awaitClear; +import static com.google.inject.internal.WeakKeySetUtils.awaitFullGc; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -34,6 +34,13 @@ import com.google.inject.Injector; import com.google.inject.Key; import com.google.inject.Scope; import com.google.inject.TypeLiteral; +import com.google.inject.internal.BindingImpl; +import com.google.inject.internal.Errors; +/*if[AOP]*/ +import com.google.inject.internal.MethodAspect; +/*end[AOP]*/ +import com.google.inject.internal.State; +import com.google.inject.internal.WeakKeySet; import com.google.inject.spi.ModuleAnnotatedMethodScannerBinding; import com.google.inject.spi.ProvisionListenerBinding; import com.google.inject.spi.ScopeBinding; diff --git a/core/test/com/google/inject/internal/WeakKeySetUtils.java b/core/test/com/google/inject/internal/WeakKeySetUtils.java index b023aa1..bab9e92 100644 --- a/core/test/com/google/inject/internal/WeakKeySetUtils.java +++ b/core/test/com/google/inject/internal/WeakKeySetUtils.java @@ -18,11 +18,15 @@ import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertNotNull; import static junit.framework.Assert.assertNull; +import static junit.framework.Assert.assertSame; import static junit.framework.Assert.assertTrue; +import com.google.common.testing.GcFinalization; import com.google.inject.Injector; import com.google.inject.Key; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; import java.util.Set; /** @@ -34,6 +38,44 @@ public final class WeakKeySetUtils { private WeakKeySetUtils() {} + public static void awaitFullGc() { + // GcFinalization *should* do it, but doesn't work well in practice... + // so we put a second latch and wait for a ReferenceQueue to tell us. + ReferenceQueue queue = new ReferenceQueue(); + WeakReference ref = new WeakReference(new Object(), queue); + GcFinalization.awaitFullGc(); + try { + assertSame("queue didn't return ref in time", ref, queue.remove(5000)); + } catch (IllegalArgumentException e) { + throw new RuntimeException(e); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + + public static void awaitClear(WeakReference ref) { + // GcFinalization *should* do it, but doesn't work well in practice... + // so we put a second latch and wait for a ReferenceQueue to tell us. + Object data = ref.get(); + ReferenceQueue queue = null; + WeakReference extraRef = null; + if (data != null) { + queue = new ReferenceQueue(); + extraRef = new WeakReference(data, queue); + data = null; + } + GcFinalization.awaitClear(ref); + if (queue != null) { + try { + assertSame("queue didn't return ref in time", extraRef, queue.remove(5000)); + } catch (IllegalArgumentException e) { + throw new RuntimeException(e); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + } + public static void assertBlacklisted(Injector injector, Key key) { assertBlacklistState(injector, key, true); } diff --git a/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java b/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java index 4206521..849993f 100644 --- a/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java +++ b/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java @@ -32,7 +32,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.inject.AbstractModule; -import com.google.inject.Asserts; import com.google.inject.Binding; import com.google.inject.BindingAnnotation; import com.google.inject.ConfigurationException; @@ -1026,7 +1025,7 @@ public class MapBinderTest extends TestCase { // Clear the ref, GC, and ensure that we are no longer blacklisting. childInjector = null; - Asserts.awaitClear(weakRef); + WeakKeySetUtils.awaitClear(weakRef); WeakKeySetUtils.assertNotBlacklisted(parentInjector, mapKey); } } diff --git a/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java b/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java index f3c9f63..d0a239a 100644 --- a/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java +++ b/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java @@ -30,7 +30,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.inject.AbstractModule; -import com.google.inject.Asserts; import com.google.inject.Binding; import com.google.inject.BindingAnnotation; import com.google.inject.CreationException; @@ -1204,7 +1203,7 @@ public class OptionalBinderTest extends TestCase { // Clear the ref, GC, and ensure that we are no longer blacklisting. childInjector = null; - Asserts.awaitClear(weakRef); + WeakKeySetUtils.awaitClear(weakRef); WeakKeySetUtils.assertNotBlacklisted(parentInjector, Key.get(Integer.class)); } -- 2.1.0