diff --git a/build.xml b/build.xml index 39ec3f7bce..c2abba1fca 100644 --- a/build.xml +++ b/build.xml @@ -289,10 +289,10 @@ ]]> - ]]> + ]]> - ]]> + ]]> diff --git a/common.xml b/common.xml index a23eb902de..257984cab3 100644 --- a/common.xml +++ b/common.xml @@ -144,7 +144,7 @@ classpath="${common.basedir}/lib/build/jarjar-1.1.jar"/> - + diff --git a/core/src/com/google/inject/internal/BytecodeGen.java b/core/src/com/google/inject/internal/BytecodeGen.java index ed01cd10f8..2d72168f5f 100644 --- a/core/src/com/google/inject/internal/BytecodeGen.java +++ b/core/src/com/google/inject/internal/BytecodeGen.java @@ -29,6 +29,7 @@ import java.lang.reflect.Modifier; import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.logging.Level; import java.util.logging.Logger; /** @@ -48,8 +49,8 @@ *

For each generated class, there's multiple class loaders involved: *

    *
  • The related class's class loader. Every generated class services exactly - * one user-supplied class. This class loader must be used to access members with private and - * package visibility. + * one user-supplied class. This class loader must be used to access members with protected + * and package visibility. *
  • Guice's class loader. *
  • Our bridge class loader. This is a child of the user's class loader. It * selectively delegates to either the user's class loader (for user classes) or the Guice @@ -191,19 +192,106 @@ private static ClassLoader getClassLoader(Class type, ClassLoader delegate) { } /*if[AOP]*/ - // use fully-qualified names so imports don't need preprocessor statements - public static net.sf.cglib.reflect.FastClass newFastClass(Class type, Visibility visibility) { + // use fully-qualified names so imports don't need preprocessor statements + /** + * Returns a FastClass proxy for invoking the given member or {@code null} if access rules + * disallow it. + * + * @see #newFastClassForMember(Class, Member) for a full description + */ + public static net.sf.cglib.reflect.FastClass newFastClassForMember(Member member) { + return newFastClassForMember(member.getDeclaringClass(), member); + } + + /** + * Returns a FastClass proxy for invoking the given member or {@code null} if access rules + * disallow it. + * + *

    FastClass works by generating a type in the same package as the target {@code type}. This + * may or may not work depending on the access level of the class/member. It breaks down into the + * following cases depending on accessibility: + *

      + *
    • Public: This always works since we can generate the type into the + * {@link BridgeClassLoader} which ensures there are no versioning issues. + *
    • Package private and Protected: This works as long as: + *
        + *
      • We can generate into the same classloader as the type. This is not possible for JDK + * types which use the 'bootstrap' loader. + *
      • The classloader of the type has the same version of {@code FastClass} as we do. This + * may be violated when running in OSGI bundles. + *
      + *
    • Private: This never works. + *
    + * + * If we are unable to generate the type, then we return null and callers should work around by + * using normal java reflection. + */ + public static net.sf.cglib.reflect.FastClass newFastClassForMember(Class type, Member member) { + if (!new net.sf.cglib.core.VisibilityPredicate(type, false).evaluate(member)) { + // the member cannot be indexed by fast class. Bail out. + return null; + } + + boolean publiclyCallable = isPubliclyCallable(member); + if (!publiclyCallable && !hasSameVersionOfCglib(type.getClassLoader())) { + // The type is in a classloader with a different version of cglib and is not publicly visible + // (so we can't use the bridge classloader to work around). Bail out. + return null; + } net.sf.cglib.reflect.FastClass.Generator generator = new net.sf.cglib.reflect.FastClass.Generator(); - generator.setType(type); - if (visibility == Visibility.PUBLIC) { + if (publiclyCallable) { + // Use the bridge classloader if we can generator.setClassLoader(getClassLoader(type)); } + generator.setType(type); generator.setNamingPolicy(FASTCLASS_NAMING_POLICY); - logger.fine("Loading " + type + " FastClass with " + generator.getClassLoader()); + if (logger.isLoggable(Level.FINE)) { + logger.fine("Loading " + type + " FastClass with " + generator.getClassLoader()); + } return generator.create(); } + /** + * Returns true if the types classloader has the same version of cglib that BytecodeGen has. This + * only returns false in strange OSGI situations, but it prevents us from using FastClass for non + * public members. + */ + private static boolean hasSameVersionOfCglib(ClassLoader classLoader) { + Class fc = net.sf.cglib.reflect.FastClass.class; + try { + return classLoader.loadClass(fc.getName()) == fc; + } catch (ClassNotFoundException e) { + return false; + } + } + + /** + * Returns true if the member can be called by a fast class generated in a different classloader. + */ + private static boolean isPubliclyCallable(Member member) { + if (!Modifier.isPublic(member.getModifiers())) { + return false; + } + Class[] parameterTypes; + if (member instanceof Constructor) { + parameterTypes = ((Constructor) member).getParameterTypes(); + } else { + Method method = (Method) member; + if (!Modifier.isPublic(method.getReturnType().getModifiers())) { + return false; + } + parameterTypes = method.getParameterTypes(); + } + + for (Class type : parameterTypes) { + if (!Modifier.isPublic(type.getModifiers())) { + return false; + } + } + return true; + } + public static net.sf.cglib.proxy.Enhancer newEnhancer(Class type, Visibility visibility) { net.sf.cglib.proxy.Enhancer enhancer = new net.sf.cglib.proxy.Enhancer(); enhancer.setSuperclass(type); diff --git a/core/src/com/google/inject/internal/DefaultConstructionProxyFactory.java b/core/src/com/google/inject/internal/DefaultConstructionProxyFactory.java index 947b49a265..e3b0fa4e30 100644 --- a/core/src/com/google/inject/internal/DefaultConstructionProxyFactory.java +++ b/core/src/com/google/inject/internal/DefaultConstructionProxyFactory.java @@ -16,8 +16,8 @@ package com.google.inject.internal; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; -import com.google.inject.internal.BytecodeGen.Visibility; import com.google.inject.spi.InjectionPoint; import java.lang.reflect.Constructor; @@ -42,66 +42,111 @@ final class DefaultConstructionProxyFactory implements ConstructionProxyFacto this.injectionPoint = injectionPoint; } + @Override public ConstructionProxy create() { @SuppressWarnings("unchecked") // the injection point is for a constructor of T final Constructor constructor = (Constructor) injectionPoint.getMember(); - // Use FastConstructor if the constructor is public. - if (Modifier.isPublic(constructor.getModifiers())) { - Class classToConstruct = constructor.getDeclaringClass(); - /*if[AOP]*/ - try { - final net.sf.cglib.reflect.FastConstructor fastConstructor - = BytecodeGen.newFastClass(classToConstruct, Visibility.forMember(constructor)) - .getConstructor(constructor); - - return new ConstructionProxy() { - @SuppressWarnings("unchecked") - public T newInstance(Object... arguments) throws InvocationTargetException { - return (T) fastConstructor.newInstance(arguments); - } - public InjectionPoint getInjectionPoint() { - return injectionPoint; - } - public Constructor getConstructor() { - return constructor; - } - public ImmutableMap> - getMethodInterceptors() { - return ImmutableMap.of(); - } - }; - } catch (net.sf.cglib.core.CodeGenerationException e) {/* fall-through */} - /*end[AOP]*/ - if (!Modifier.isPublic(classToConstruct.getModifiers())) { - constructor.setAccessible(true); + /*if[AOP]*/ + try { + net.sf.cglib.reflect.FastClass fc = + BytecodeGen.newFastClassForMember(constructor); + if (fc != null) { + int index = fc.getIndex(constructor.getParameterTypes()); + // We could just fall back to reflection in this case but I believe this should actually + // be impossible. + Preconditions.checkArgument(index >= 0, + "Could not find constructor %s in fast class", + constructor); + return new FastClassProxy(injectionPoint, constructor, fc, index); } - } else { - constructor.setAccessible(true); + } catch (net.sf.cglib.core.CodeGenerationException e) {/* fall-through */} + /*end[AOP]*/ + + return new ReflectiveProxy(injectionPoint, constructor); + } + + /*if[AOP]*/ + /** A {@link ConstructionProxy} that uses FastClass to invoke the constructor. */ + private static final class FastClassProxy implements ConstructionProxy { + final InjectionPoint injectionPoint; + final Constructor constructor; + final net.sf.cglib.reflect.FastClass fc; + final int index; + + private FastClassProxy(InjectionPoint injectionPoint, Constructor constructor, + net.sf.cglib.reflect.FastClass fc, int index) { + this.injectionPoint = injectionPoint; + this.constructor = constructor; + this.fc = fc; + this.index = index; } - return new ConstructionProxy() { - public T newInstance(Object... arguments) throws InvocationTargetException { - try { - return constructor.newInstance(arguments); - } catch (InstantiationException e) { - throw new AssertionError(e); // shouldn't happen, we know this is a concrete type - } catch (IllegalAccessException e) { - throw new AssertionError(e); // a security manager is blocking us, we're hosed - } - } - public InjectionPoint getInjectionPoint() { - return injectionPoint; - } - public Constructor getConstructor() { - return constructor; + @Override + @SuppressWarnings("unchecked") + public T newInstance(Object... arguments) throws InvocationTargetException { + // Use this method instead of FastConstructor to save a stack frame + return (T) fc.newInstance(index, arguments); + } + + @Override + public InjectionPoint getInjectionPoint() { + return injectionPoint; + } + + @Override + public Constructor getConstructor() { + return constructor; + } + + @Override + public ImmutableMap> + getMethodInterceptors() { + return ImmutableMap.of(); + } + } + /*end[AOP]*/ + + private static final class ReflectiveProxy implements ConstructionProxy { + final Constructor constructor; + final InjectionPoint injectionPoint; + + ReflectiveProxy(InjectionPoint injectionPoint, Constructor constructor) { + if (!Modifier.isPublic(constructor.getDeclaringClass().getModifiers()) + || !Modifier.isPublic(constructor.getModifiers())) { + constructor.setAccessible(true); } - /*if[AOP]*/ - public ImmutableMap> - getMethodInterceptors() { - return ImmutableMap.of(); + this.injectionPoint = injectionPoint; + this.constructor = constructor; + } + + @Override + public T newInstance(Object... arguments) throws InvocationTargetException { + try { + return constructor.newInstance(arguments); + } catch (InstantiationException e) { + throw new AssertionError(e); // shouldn't happen, we know this is a concrete type + } catch (IllegalAccessException e) { + throw new AssertionError(e); // a security manager is blocking us, we're hosed } - /*end[AOP]*/ - }; + } + + @Override + public InjectionPoint getInjectionPoint() { + return injectionPoint; + } + + @Override + public Constructor getConstructor() { + return constructor; + } + + /*if[AOP]*/ + @Override + public ImmutableMap> + getMethodInterceptors() { + return ImmutableMap.of(); + } + /*end[AOP]*/ } } diff --git a/core/src/com/google/inject/internal/ProviderMethod.java b/core/src/com/google/inject/internal/ProviderMethod.java index beaf406e31..3255b632be 100644 --- a/core/src/com/google/inject/internal/ProviderMethod.java +++ b/core/src/com/google/inject/internal/ProviderMethod.java @@ -17,7 +17,6 @@ package com.google.inject.internal; import com.google.common.base.Objects; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.inject.Binder; import com.google.inject.Exposed; @@ -25,7 +24,6 @@ import com.google.inject.PrivateBinder; import com.google.inject.Provider; import com.google.inject.Provides; -import com.google.inject.internal.BytecodeGen.Visibility; import com.google.inject.internal.util.StackTraceElements; import com.google.inject.spi.BindingTargetVisitor; import com.google.inject.spi.Dependency; @@ -64,17 +62,19 @@ static ProviderMethod create(Key key, Method method, Object instance, Annotation annotation) { int modifiers = method.getModifiers(); /*if[AOP]*/ - if (!skipFastClassGeneration && !Modifier.isPrivate(modifiers) - && !Modifier.isProtected(modifiers)) { + if (!skipFastClassGeneration) { try { - // We use an index instead of FastMethod to save a stack frame. - return new FastClassProviderMethod(key, - method, - instance, - dependencies, - parameterProviders, - scopeAnnotation, - annotation); + net.sf.cglib.reflect.FastClass fc = BytecodeGen.newFastClassForMember(method); + if (fc != null) { + return new FastClassProviderMethod(key, + fc, + method, + instance, + dependencies, + parameterProviders, + scopeAnnotation, + annotation); + } } catch (net.sf.cglib.core.CodeGenerationException e) {/* fall-through */} } /*end[AOP]*/ @@ -239,6 +239,7 @@ private static final class FastClassProviderMethod extends ProviderMethod final int methodIndex; FastClassProviderMethod(Key key, + net.sf.cglib.reflect.FastClass fc, Method method, Object instance, ImmutableSet> dependencies, @@ -252,19 +253,8 @@ private static final class FastClassProviderMethod extends ProviderMethod parameterProviders, scopeAnnotation, annotation); - // We need to generate a FastClass for the method's class, not the object's class. - this.fastClass = - BytecodeGen.newFastClass(method.getDeclaringClass(), Visibility.forMember(method)); - // Use the Signature overload of getIndex because it properly uses return types to identify - // particular methods. This is normally irrelevant, except in the case of covariant overrides - // which java implements with a compiler generated bridge method to implement the override. - this.methodIndex = fastClass.getIndex( - new net.sf.cglib.core.Signature( - method.getName(), org.objectweb.asm.Type.getMethodDescriptor(method))); - Preconditions.checkArgument(this.methodIndex >= 0, - "Could not find method %s in fast class for class %s", - method, - method.getDeclaringClass()); + this.fastClass = fc; + this.methodIndex = fc.getMethod(method).getIndex(); } @Override public Object doProvision(Object[] parameters) diff --git a/core/src/com/google/inject/internal/ProxyFactory.java b/core/src/com/google/inject/internal/ProxyFactory.java index 13aff81055..40c2146e1a 100644 --- a/core/src/com/google/inject/internal/ProxyFactory.java +++ b/core/src/com/google/inject/internal/ProxyFactory.java @@ -16,7 +16,7 @@ package com.google.inject.internal; -import static com.google.inject.internal.BytecodeGen.newFastClass; +import static com.google.inject.internal.BytecodeGen.newFastClassForMember; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -30,7 +30,6 @@ import net.sf.cglib.proxy.CallbackFilter; import net.sf.cglib.proxy.Enhancer; import net.sf.cglib.reflect.FastClass; -import net.sf.cglib.reflect.FastConstructor; import org.aopalliance.intercept.MethodInterceptor; @@ -243,8 +242,9 @@ private static class ProxyConstructor implements ConstructionProxy { final Constructor constructor; final Callback[] callbacks; - final FastConstructor fastConstructor; + final int constructorIndex; final ImmutableMap> methodInterceptors; + final FastClass fastClass; @SuppressWarnings("unchecked") // the constructor promises to construct 'T's ProxyConstructor(Enhancer enhancer, InjectionPoint injectionPoint, Callback[] callbacks, @@ -254,16 +254,15 @@ private static class ProxyConstructor implements ConstructionProxy { this.constructor = (Constructor) injectionPoint.getMember(); this.callbacks = callbacks; this.methodInterceptors = methodInterceptors; - - FastClass fastClass = newFastClass(enhanced, BytecodeGen.Visibility.forMember(constructor)); - this.fastConstructor = fastClass.getConstructor(constructor.getParameterTypes()); + this.fastClass = newFastClassForMember(enhanced, constructor); + this.constructorIndex = fastClass.getIndex(constructor.getParameterTypes()); } @SuppressWarnings("unchecked") // the constructor promises to produce 'T's public T newInstance(Object... arguments) throws InvocationTargetException { Enhancer.registerCallbacks(enhanced, callbacks); try { - return (T) fastConstructor.newInstance(arguments); + return (T) fastClass.newInstance(constructorIndex, arguments); } finally { Enhancer.registerCallbacks(enhanced, null); } diff --git a/core/src/com/google/inject/internal/SingleMethodInjector.java b/core/src/com/google/inject/internal/SingleMethodInjector.java index 8bc8a34eae..2218efd59f 100644 --- a/core/src/com/google/inject/internal/SingleMethodInjector.java +++ b/core/src/com/google/inject/internal/SingleMethodInjector.java @@ -16,7 +16,6 @@ package com.google.inject.internal; -import com.google.inject.internal.BytecodeGen.Visibility; import com.google.inject.internal.InjectorImpl.MethodInvoker; import com.google.inject.spi.InjectionPoint; @@ -42,25 +41,24 @@ final class SingleMethodInjector implements SingleMemberInjector { private MethodInvoker createMethodInvoker(final Method method) { - // We can't use FastMethod if the method is private. - int modifiers = method.getModifiers(); - if (!Modifier.isPrivate(modifiers) && !Modifier.isProtected(modifiers)) { - /*if[AOP]*/ - try { - final net.sf.cglib.reflect.FastMethod fastMethod - = BytecodeGen.newFastClass(method.getDeclaringClass(), Visibility.forMember(method)) - .getMethod(method); + /*if[AOP]*/ + try { + final net.sf.cglib.reflect.FastClass fastClass = + BytecodeGen.newFastClassForMember(method); + if (fastClass != null) { + final int index = fastClass.getMethod(method).getIndex(); - return new MethodInvoker() { - public Object invoke(Object target, Object... parameters) - throws IllegalAccessException, InvocationTargetException { - return fastMethod.invoke(target, parameters); - } - }; - } catch (net.sf.cglib.core.CodeGenerationException e) {/* fall-through */} - /*end[AOP]*/ - } + return new MethodInvoker() { + public Object invoke(Object target, Object... parameters) + throws IllegalAccessException, InvocationTargetException { + return fastClass.invoke(index, target, parameters); + } + }; + } + } catch (net.sf.cglib.core.CodeGenerationException e) {/* fall-through */} + /*end[AOP]*/ + int modifiers = method.getModifiers(); if (!Modifier.isPublic(modifiers) || !Modifier.isPublic(method.getDeclaringClass().getModifiers())) { method.setAccessible(true); diff --git a/core/test/com/google/inject/ImplicitBindingTest.java b/core/test/com/google/inject/ImplicitBindingTest.java index eb47e49fc6..188f19ac8a 100644 --- a/core/test/com/google/inject/ImplicitBindingTest.java +++ b/core/test/com/google/inject/ImplicitBindingTest.java @@ -392,4 +392,12 @@ public void testImplementedByEnum() { @ImplementedBy(EnumWithImplementedByEnum.class) enum EnumWithImplementedBy {} private static class EnumWithImplementedByEnum {} + + public void testImplicitJdkBindings() { + Injector injector = Guice.createInjector(); + // String has a public nullary constructor, so Guice will call it. + assertEquals("", injector.getInstance(String.class)); + // InetAddress has a package private constructor. We probably shouldn't be calling it :( + assertNotNull(injector.getInstance(java.net.InetAddress.class)); + } } diff --git a/core/test/com/google/inject/MethodInterceptionTest.java b/core/test/com/google/inject/MethodInterceptionTest.java index de39bd9597..afa3961448 100644 --- a/core/test/com/google/inject/MethodInterceptionTest.java +++ b/core/test/com/google/inject/MethodInterceptionTest.java @@ -141,7 +141,7 @@ public Object invoke(MethodInvocation methodInvocation) throws Throwable { } catch(ConfigurationException ce) { assertEquals("Unable to method intercept: " + NotInterceptable.class.getName(), Iterables.getOnlyElement(ce.getErrorMessages()).getMessage().toString()); - assertEquals("Cannot subclass final class class " + NotInterceptable.class.getName(), + assertEquals("Cannot subclass final class " + NotInterceptable.class.getName(), ce.getCause().getMessage()); } } diff --git a/core/test/com/googlecode/guice/BytecodeGenTest.java b/core/test/com/googlecode/guice/BytecodeGenTest.java index de4d560ded..c289fd5eb0 100644 --- a/core/test/com/googlecode/guice/BytecodeGenTest.java +++ b/core/test/com/googlecode/guice/BytecodeGenTest.java @@ -23,6 +23,8 @@ import com.google.inject.Guice; import com.google.inject.Injector; import com.google.inject.Module; + +import com.googlecode.guice.BytecodeGenTest.LogCreator; import com.googlecode.guice.PackageVisibilityTestModule.PublicUserOfPackagePrivate; import junit.framework.TestCase; @@ -30,15 +32,13 @@ import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; -import java.io.File; import java.lang.ref.Reference; import java.lang.ref.WeakReference; import java.lang.reflect.Constructor; import java.lang.reflect.Method; -import java.net.MalformedURLException; -import java.net.URL; import java.net.URLClassLoader; -import java.util.concurrent.TimeoutException; + +import javax.inject.Inject; /** * This test is in a separate package so we can test package-level visibility @@ -97,27 +97,15 @@ public void testEnhancerNaming() { static class TestVisibilityClassLoader extends URLClassLoader { - boolean hideInternals; + final boolean hideInternals; - public TestVisibilityClassLoader(boolean hideInternals) { - super(new URL[0]); + TestVisibilityClassLoader(boolean hideInternals) { + this((URLClassLoader) TestVisibilityClassLoader.class.getClassLoader(), hideInternals); + } + TestVisibilityClassLoader(URLClassLoader classloader, boolean hideInternals) { + super(classloader.getURLs(), classloader); this.hideInternals = hideInternals; - - final String[] classpath = System.getProperty("java.class.path").split(File.pathSeparator); - for (final String element : classpath) { - try { - // is it a remote/local URL? - addURL(new URL(element)); - } catch (final MalformedURLException e1) { - try { - // nope - perhaps it's a filename? - addURL(new File(element).toURI().toURL()); - } catch (final MalformedURLException e2) { - throw new RuntimeException(e1); - } - } - } } /** @@ -193,7 +181,7 @@ interface ProxyTest { * Note: this class must be marked as public or protected so that the Guice * custom classloader will intercept it. Private and implementation classes * are not intercepted by the custom classloader. - * + * * @see com.google.inject.internal.BytecodeGen.Visibility */ public static class ProxyTestImpl implements ProxyTest { @@ -325,4 +313,131 @@ public void testClassLoaderBridging() throws Exception { Object o2 = injector.getInstance(hiddenMethodReturnClass); o2.getClass().getDeclaredMethod("method").invoke(o2); } + + // This tests for a situation where a osgi bundle contains a version of guice. When guice + // generates a fast class it will use a bridge classloader + public void testFastClassUsesBridgeClassloader() throws Throwable { + Injector injector = Guice.createInjector(); + // These classes are all in the same classloader as guice itself, so other than the private one + // they can all be fast class invoked + injector.getInstance(PublicInject.class).assertIsFastClassInvoked(); + injector.getInstance(ProtectedInject.class).assertIsFastClassInvoked(); + injector.getInstance(PackagePrivateInject.class).assertIsFastClassInvoked(); + injector.getInstance(PrivateInject.class).assertIsReflectionInvoked(); + + // This classloader will load the types in an loader with a different version of guice/cglib + // this prevents the use of fastclass for all but the public types (where the bridge + // classloader can be used). + MultipleVersionsOfGuiceClassLoader fakeLoader = new MultipleVersionsOfGuiceClassLoader(); + injector.getInstance(fakeLoader.loadLogCreatorType(PublicInject.class)) + .assertIsFastClassInvoked(); + injector.getInstance(fakeLoader.loadLogCreatorType(ProtectedInject.class)) + .assertIsReflectionInvoked(); + injector.getInstance(fakeLoader.loadLogCreatorType(PackagePrivateInject.class)) + .assertIsReflectionInvoked(); + injector.getInstance(fakeLoader.loadLogCreatorType(PrivateInject.class)) + .assertIsReflectionInvoked(); + } + + // This classloader simulates an OSGI environment where a bundle has a conflicting definition of + // cglib (or guice). This is sort of the opposite of the BridgeClassloader and is meant to test + // its use. + static class MultipleVersionsOfGuiceClassLoader extends URLClassLoader { + MultipleVersionsOfGuiceClassLoader() { + this((URLClassLoader) MultipleVersionsOfGuiceClassLoader.class.getClassLoader()); + } + + MultipleVersionsOfGuiceClassLoader(URLClassLoader classloader) { + super(classloader.getURLs(), classloader); + } + + public Class loadLogCreatorType(Class cls) + throws ClassNotFoundException { + return loadClass(cls.getName()).asSubclass(LogCreator.class); + } + + /** + * Classic parent-delegating classloaders are meant to override findClass. + * However, non-delegating classloaders (as used in OSGi) instead override + * loadClass to provide support for "class-space" separation. + */ + @Override + protected Class loadClass(final String name, final boolean resolve) + throws ClassNotFoundException { + + synchronized (this) { + // check our local cache to avoid duplicates + final Class clazz = findLoadedClass(name); + if (clazz != null) { + return clazz; + } + } + + if (name.startsWith("java.") + || name.startsWith("javax.") + || name.equals(LogCreator.class.getName()) + || (!name.startsWith("com.google.inject.") + && !name.contains(".cglib.") + && !name.startsWith("com.googlecode.guice"))) { + + // standard parent delegation + return super.loadClass(name, resolve); + + } else { + // load a new copy of the class + final Class clazz = findClass(name); + if (resolve) { + resolveClass(clazz); + } + return clazz; + } + } + } + + public static class LogCreator { + final Throwable caller; + public LogCreator() { + this.caller = new Throwable(); + } + + void assertIsFastClassInvoked() throws Throwable { + // 2 because the first 2 elements are + // LogCreator.() + // Subclass.() + if (!caller.getStackTrace()[2].getClassName().contains("$$FastClassByGuice$$")) { + throw new AssertionError("Caller was not FastClass").initCause(caller); + } + } + + void assertIsReflectionInvoked() throws Throwable { + // Scan for a call to Constructor.newInstance, but stop if we see the test itself. + for (StackTraceElement element : caller.getStackTrace()) { + if (element.getClassName().equals(BytecodeGenTest.class.getName())) { + // break when we hit the test method. + break; + } + if (element.getClassName().equals(Constructor.class.getName()) + && element.getMethodName().equals("newInstance")) { + return; + } + } + throw new AssertionError("Caller was not Constructor.newInstance").initCause(caller); + } + } + + public static class PublicInject extends LogCreator { + @Inject public PublicInject() {} + } + + static class PackagePrivateInject extends LogCreator { + @Inject PackagePrivateInject() {} + } + + protected static class ProtectedInject extends LogCreator { + @Inject protected ProtectedInject() {} + } + + private static class PrivateInject extends LogCreator { + @Inject private PrivateInject() {} + } } diff --git a/extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java b/extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java index ba7a5afad9..f80a860881 100644 --- a/extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java +++ b/extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java @@ -25,6 +25,8 @@ import java.io.IOException; import java.lang.ref.WeakReference; import java.util.concurrent.Callable; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Logger; import javax.servlet.Filter; @@ -127,13 +129,13 @@ public void doFilter( HttpServletRequest originalRequest = (previous != null) ? previous.getOriginalRequest() : request; try { - new Context(originalRequest, request, response).call(new Callable() { - @Override public Void call() throws Exception { - //dispatch across the servlet pipeline, ensuring web.xml's filterchain is honored - filterPipeline.dispatch(servletRequest, servletResponse, filterChain); - return null; - } - }); + RequestScoper.CloseableScope scope = new Context(originalRequest, request, response).open(); + try { + //dispatch across the servlet pipeline, ensuring web.xml's filterchain is honored + filterPipeline.dispatch(servletRequest, servletResponse, filterChain); + } finally { + scope.close(); + } } catch (IOException e) { throw e; } catch (ServletException e) { @@ -170,11 +172,15 @@ private static Context getContext(Key key) { return context; } - static class Context { + static class Context implements RequestScoper { final HttpServletRequest originalRequest; final HttpServletRequest request; final HttpServletResponse response; + // Synchronized to prevent two threads from using the same request + // scope concurrently. + final Lock lock = new ReentrantLock(); + Context(HttpServletRequest originalRequest, HttpServletRequest request, HttpServletResponse response) { this.originalRequest = originalRequest; @@ -194,16 +200,16 @@ HttpServletResponse getResponse() { return response; } - // Synchronized to prevent two threads from using the same request - // scope concurrently. - synchronized T call(Callable callable) throws Exception { - Context previous = localContext.get(); + @Override public CloseableScope open() { + lock.lock(); + final Context previous = localContext.get(); localContext.set(this); - try { - return callable.call(); - } finally { - localContext.set(previous); - } + return new CloseableScope() { + @Override public void close() { + localContext.set(previous); + lock.unlock(); + } + }; } } diff --git a/extensions/servlet/src/com/google/inject/servlet/RequestScoper.java b/extensions/servlet/src/com/google/inject/servlet/RequestScoper.java new file mode 100644 index 0000000000..74082b853b --- /dev/null +++ b/extensions/servlet/src/com/google/inject/servlet/RequestScoper.java @@ -0,0 +1,21 @@ +package com.google.inject.servlet; + +import java.io.Closeable; +import java.util.concurrent.Callable; + +/** Object that can be used to apply a request scope to a block of code. */ +public interface RequestScoper { + /** + * Opens up the request scope until the returned object is closed. + * Implementations should ensure (e.g. by blocking) that multiple threads + * cannot open the same request scope concurrently. It is allowable to open + * the same request scope on the same thread, as long as open/close calls are + * correctly nested. + */ + CloseableScope open(); + + /** Closeable subclass that does not throw any exceptions from close. */ + public interface CloseableScope extends Closeable { + @Override void close(); + } +} diff --git a/extensions/servlet/src/com/google/inject/servlet/ServletScopes.java b/extensions/servlet/src/com/google/inject/servlet/ServletScopes.java index ced5288eae..c59061321a 100644 --- a/extensions/servlet/src/com/google/inject/servlet/ServletScopes.java +++ b/extensions/servlet/src/com/google/inject/servlet/ServletScopes.java @@ -30,6 +30,8 @@ import java.util.Map; import java.util.concurrent.Callable; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -218,9 +220,15 @@ public String toString() { * thread, or if the request has completed. * * @since 3.0 + * @deprecated You probably want to use {@code transferRequest} instead */ - public static Callable continueRequest(final Callable callable, - final Map, Object> seedMap) { + @Deprecated + public static Callable continueRequest(Callable callable, + Map, Object> seedMap) { + return wrap(callable, continueRequest(seedMap)); + } + + private static RequestScoper continueRequest(Map, Object> seedMap) { Preconditions.checkArgument(null != seedMap, "Seed map cannot be null, try passing in Collections.emptyMap() instead."); @@ -233,12 +241,11 @@ public static Callable continueRequest(final Callable callable, continuingRequest.setAttribute(entry.getKey().toString(), value); } - return new Callable() { - public T call() throws Exception { + return new RequestScoper() { + @Override public CloseableScope open() { checkScopingState(null == GuiceFilter.localContext.get(), "Cannot continue request in the same thread as a HTTP request!"); - return new GuiceFilter.Context(continuingRequest, continuingRequest, null) - .call(callable); + return new GuiceFilter.Context(continuingRequest, continuingRequest, null).open(); } }; } @@ -267,33 +274,51 @@ public T call() throws Exception { * @since 4.0 */ public static Callable transferRequest(Callable callable) { + return wrap(callable, transferRequest()); + } + + /** + * Returns an object that "transfers" the request to another thread. This acts + * as a way of transporting request context data from the current thread to a + * future thread. The transferred scope is the one active for the thread that + * calls this method. A later call to {@code open()} activates the transferred + * the scope, including propagating any objects scoped at that time. + * + *

    As opposed to {@link #continueRequest}, this method propagates all + * existing scoped objects. The primary use case is in server implementations + * where you can detach the request processing thread while waiting for data, + * and reattach to a different thread to finish processing at a later time. + * + *

    Because request-scoped objects are not typically thread-safe, it is + * important to avoid applying the same request scope concurrently. The + * returned Scoper will block on open until the current thread has released + * the request scope. + * + * @return an object that when opened will initiate the request scope + * @throws OutOfScopeException if this method is called from a non-request + * thread, or if the request has completed. + * @since 5.0 + */ + public static RequestScoper transferRequest() { return (GuiceFilter.localContext.get() != null) - ? transferHttpRequest(callable) - : transferNonHttpRequest(callable); + ? transferHttpRequest() + : transferNonHttpRequest(); } - private static Callable transferHttpRequest(final Callable callable) { + private static RequestScoper transferHttpRequest() { final GuiceFilter.Context context = GuiceFilter.localContext.get(); if (context == null) { throw new OutOfScopeException("Not in a request scope"); } - return new Callable() { - public T call() throws Exception { - return context.call(callable); - } - }; + return context; } - private static Callable transferNonHttpRequest(final Callable callable) { + private static RequestScoper transferNonHttpRequest() { final Context context = requestScopeContext.get(); if (context == null) { throw new OutOfScopeException("Not in a request scope"); } - return new Callable() { - public T call() throws Exception { - return context.call(callable); - } - }; + return context; } /** @@ -328,8 +353,28 @@ public static boolean isRequestScoped(Binding binding) { * that exposes the instances in the {@code seedMap} as scoped keys. * @since 3.0 */ - public static Callable scopeRequest(final Callable callable, + public static Callable scopeRequest(Callable callable, Map, Object> seedMap) { + return wrap(callable, scopeRequest(seedMap)); + } + + /** + * Returns an object that will apply request scope to a block of code. This is + * not the same as the HTTP request scope, but is used if no HTTP request + * scope is in progress. In this way, keys can be scoped as @RequestScoped and + * exist in non-HTTP requests (for example: RPC requests) as well as in HTTP + * request threads. + * + *

    The returned object will throw a {@link ScopingException} when opened + * if there is a request scope already active on the current thread. + * + * @param seedMap the initial set of scoped instances for Guice to seed the + * request scope with. To seed a key with null, use {@code null} as + * the value. + * @return an object that when opened will initiate the request scope + * @since 5.0 + */ + public static RequestScoper scopeRequest(Map, Object> seedMap) { Preconditions.checkArgument(null != seedMap, "Seed map cannot be null, try passing in Collections.emptyMap() instead."); @@ -342,14 +387,13 @@ public static Callable scopeRequest(final Callable callable, } }); context.map.putAll(validatedAndCanonicalizedMap); - - return new Callable() { - public T call() throws Exception { + return new RequestScoper() { + @Override public CloseableScope open() { checkScopingState(null == GuiceFilter.localContext.get(), "An HTTP request is already in progress, cannot scope a new request in this thread."); checkScopingState(null == requestScopeContext.get(), "A request scope is already in progress, cannot scope a new request in this thread."); - return context.call(callable); + return context.open(); } }; } @@ -371,19 +415,23 @@ private static Object validateAndCanonicalizeValue(Key key, Object object) { return object; } - private static class Context { + private static class Context implements RequestScoper { final Map map = Maps.newHashMap(); // Synchronized to prevent two threads from using the same request // scope concurrently. - synchronized T call(Callable callable) throws Exception { - Context previous = requestScopeContext.get(); + final Lock lock = new ReentrantLock(); + + @Override public CloseableScope open() { + lock.lock(); + final Context previous = requestScopeContext.get(); requestScopeContext.set(this); - try { - return callable.call(); - } finally { - requestScopeContext.set(previous); - } + return new CloseableScope() { + @Override public void close() { + requestScopeContext.set(previous); + lock.unlock(); + } + }; } } @@ -392,4 +440,19 @@ private static void checkScopingState(boolean condition, String msg) { throw new ScopingException(msg); } } + + private static final Callable wrap( + final Callable delegate, final RequestScoper requestScoper) { + return new Callable() { + public T call() throws Exception { + RequestScoper.CloseableScope scope = requestScoper.open(); + try { + return delegate.call(); + } finally { + scope.close(); + } + } + }; + } + } diff --git a/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java b/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java index dbd18f774c..edd83fbc33 100644 --- a/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java +++ b/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java @@ -59,6 +59,13 @@ public void testTransferNonHttp_outOfScope() { } catch (OutOfScopeException expected) {} } + public void testTransferNonHttp_outOfScope_closeable() { + try { + ServletScopes.transferRequest(); + fail(); + } catch (OutOfScopeException expected) {} + } + public void testTransferNonHttpRequest() throws Exception { final Injector injector = Guice.createInjector(new AbstractModule() { @Override protected void configure() { @@ -89,6 +96,44 @@ public void testTransferNonHttpRequest() throws Exception { executor.shutdownNow(); } + public void testTransferNonHttpRequest_closeable() throws Exception { + final Injector injector = Guice.createInjector(new AbstractModule() { + @Override protected void configure() { + bindScope(RequestScoped.class, ServletScopes.REQUEST); + } + + @Provides @RequestScoped Object provideObject() { + return new Object(); + } + }); + + class Data { + Object object; + RequestScoper scoper; + } + + Callable callable = new Callable() { + @Override public Data call() { + Data data = new Data(); + data.object = injector.getInstance(Object.class); + data.scoper = ServletScopes.transferRequest(); + return data; + } + }; + + ImmutableMap, Object> seedMap = ImmutableMap.of(); + Data data = ServletScopes.scopeRequest(callable, seedMap).call(); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + RequestScoper.CloseableScope scope = data.scoper.open(); + try { + assertSame(data.object, injector.getInstance(Object.class)); + } finally { + scope.close(); + executor.shutdownNow(); + } + } + public void testTransferNonHttpRequest_concurrentUseBlocks() throws Exception { Callable callable = new Callable() { @Override public Boolean call() throws Exception { @@ -110,6 +155,37 @@ public void testTransferNonHttpRequest_concurrentUseBlocks() throws Exception { assertTrue(ServletScopes.scopeRequest(callable, seedMap).call()); } + public void testTransferNonHttpRequest_concurrentUseBlocks_closeable() throws Exception { + Callable callable = new Callable() { + @Override public Boolean call() throws Exception { + final RequestScoper scoper = ServletScopes.transferRequest(); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future future = executor.submit(new Callable() { + @Override public Boolean call() { + RequestScoper.CloseableScope scope = scoper.open(); + try { + return false; + } finally { + scope.close(); + } + } + }); + try { + return future.get(100, TimeUnit.MILLISECONDS); + } catch (TimeoutException e) { + return true; + } + } finally { + executor.shutdownNow(); + } + } + }; + + ImmutableMap, Object> seedMap = ImmutableMap.of(); + assertTrue(ServletScopes.scopeRequest(callable, seedMap).call()); + } + public void testTransferNonHttpRequest_concurrentUseSameThreadOk() throws Exception { Callable callable = new Callable() { @Override public Boolean call() throws Exception { @@ -120,4 +196,20 @@ public void testTransferNonHttpRequest_concurrentUseSameThreadOk() throws Except ImmutableMap, Object> seedMap = ImmutableMap.of(); assertFalse(ServletScopes.scopeRequest(callable, seedMap).call()); } + + public void testTransferNonHttpRequest_concurrentUseSameThreadOk_closeable() throws Exception { + Callable callable = new Callable() { + @Override public Boolean call() throws Exception { + RequestScoper.CloseableScope scope = ServletScopes.transferRequest().open(); + try { + return false; + } finally { + scope.close(); + } + } + }; + + ImmutableMap, Object> seedMap = ImmutableMap.of(); + assertFalse(ServletScopes.scopeRequest(callable, seedMap).call()); + } } diff --git a/lib/build/cglib-3.1.jar b/lib/build/cglib-3.1.jar deleted file mode 100644 index 25a5df155a..0000000000 Binary files a/lib/build/cglib-3.1.jar and /dev/null differ diff --git a/lib/build/cglib-3.2.jar b/lib/build/cglib-3.2.jar new file mode 100644 index 0000000000..1ea2f0064c Binary files /dev/null and b/lib/build/cglib-3.2.jar differ diff --git a/pom.xml b/pom.xml index 5c59280bdc..5680ac8d4a 100644 --- a/pom.xml +++ b/pom.xml @@ -159,7 +159,7 @@ See the Apache License Version 2.0 for the specific language governing permissio cglib cglib - 3.1 + 3.2.0