Migrated attachment for Guice issue 343, comment 23

GUICE_ISSUE_343_20100218.txt

Index: src/com/google/inject/internal/DefaultConstructionProxyFactory.java
===================================================================
--- src/com/google/inject/internal/DefaultConstructionProxyFactory.java	(revision 1142)
+++ src/com/google/inject/internal/DefaultConstructionProxyFactory.java	(working copy)
@@ -24,6 +24,8 @@
 import java.lang.reflect.Modifier;
 import java.util.List;
 
+import net.sf.cglib.core.CodeGenerationException;
+
 /**
  * Produces construction proxies that invoke the class constructor.
  *
@@ -47,6 +49,7 @@
     // Use FastConstructor if the constructor is public.
     if (Modifier.isPublic(constructor.getModifiers())) {
       /*if[AOP]*/
+      try {
       return new ConstructionProxy<T>() {
         Class<T> classToConstruct = constructor.getDeclaringClass();
         final net.sf.cglib.reflect.FastConstructor fastConstructor
@@ -68,6 +71,10 @@
           return ImmutableMap.of();
         }
       };
+      } catch (CodeGenerationException e) {
+        // FastConstructor failed: make accessible before reverting to JDK reflection
+        constructor.setAccessible(true);
+      }
       /*end[AOP]*/
     } else {
       constructor.setAccessible(true);
Index: src/com/google/inject/internal/ProxyFactory.java
===================================================================
--- src/com/google/inject/internal/ProxyFactory.java	(revision 1142)
+++ src/com/google/inject/internal/ProxyFactory.java	(working copy)
@@ -17,6 +17,8 @@
 package com.google.inject.internal;
 
 import static com.google.inject.internal.BytecodeGen.newFastClass;
+
+import com.google.inject.ProvisionException;
 import com.google.inject.spi.InjectionPoint;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
@@ -153,11 +155,15 @@
 
     // Create the proxied class. We're careful to ensure that all enhancer state is not-specific
     // to this injector. Otherwise, the proxies for each injector will waste PermGen memory
+    try {
     Enhancer enhancer = BytecodeGen.newEnhancer(declaringClass, visibility);
     enhancer.setCallbackFilter(new IndicesCallbackFilter(declaringClass, methods));
     enhancer.setCallbackTypes(callbackTypes);
     return new ProxyConstructor<T>(enhancer, injectionPoint, callbacks, interceptors);
+    } catch (Throwable e) {
+      throw new ProvisionException("Unable to method intercept: " + declaringClass, e);
   }
+  }
 
   private static class MethodInterceptorsPair {
     final Method method;
Index: src/com/google/inject/internal/SingleMethodInjector.java
===================================================================
--- src/com/google/inject/internal/SingleMethodInjector.java	(revision 1142)
+++ src/com/google/inject/internal/SingleMethodInjector.java	(working copy)
@@ -23,6 +23,8 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 
+import net.sf.cglib.core.CodeGenerationException;
+
 /**
  * Invokes an injectable method.
  */
@@ -45,6 +47,7 @@
     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);
@@ -55,6 +58,10 @@
           return fastMethod.invoke(target, parameters);
         }
       };
+      } catch (CodeGenerationException e) {
+        // FastMethod failed: make accessible before reverting to JDK reflection
+        method.setAccessible(true);
+      }
       /*end[AOP]*/
     }
 
Index: src/com/google/inject/internal/BytecodeGen.java
===================================================================
--- src/com/google/inject/internal/BytecodeGen.java	(revision 1142)
+++ src/com/google/inject/internal/BytecodeGen.java	(working copy)
@@ -16,7 +16,6 @@
 
 package com.google.inject.internal;
 
-import static com.google.inject.internal.Preconditions.checkNotNull;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Member;
 import java.lang.reflect.Method;
@@ -57,17 +56,22 @@
  */
 final class BytecodeGen {
 
-  private static final Logger logger = Logger.getLogger(BytecodeGen.class.getName());
+  static final Logger logger = Logger.getLogger(BytecodeGen.class.getName());
 
-  static final ClassLoader GUICE_CLASS_LOADER = BytecodeGen.class.getClassLoader();
+  static final ClassLoader GUICE_CLASS_LOADER = canonicalize(BytecodeGen.class.getClassLoader());
 
+  // initialization-on-demand...
+  private static class SystemBridgeHolder {
+    static final BridgeClassLoader SYSTEM_BRIDGE = new BridgeClassLoader();
+  }
+
   /** ie. "com.google.inject.internal" */
-  private static final String GUICE_INTERNAL_PACKAGE
+  static final String GUICE_INTERNAL_PACKAGE
       = BytecodeGen.class.getName().replaceFirst("\\.internal\\..*$", ".internal");
 
   /*if[AOP]*/
   /** either "net.sf.cglib", or "com.google.inject.internal.cglib" */
-  private static final String CGLIB_PACKAGE
+  static final String CGLIB_PACKAGE
       = net.sf.cglib.proxy.Enhancer.class.getName().replaceFirst("\\.cglib\\..*$", ".cglib");
 
   static final net.sf.cglib.core.NamingPolicy NAMING_POLICY
@@ -82,8 +86,8 @@
   end[NO_AOP]*/
 
   /** Use "-Dguice.custom.loader=false" to disable custom classloading. */
-  static final boolean HOOK_ENABLED
-      = "true".equals(System.getProperty("guice.custom.loader", "true"));
+  private static final boolean CUSTOM_LOADER_ENABLED
+      = Boolean.parseBoolean(System.getProperty("guice.custom.loader", "true"));
 
   /**
    * Weak cache of bridge class loaders that make the Guice implementation
@@ -103,29 +107,14 @@
   });
 
   /**
-   * For class loaders, {@code null}, is always an alias to the
-   * {@link ClassLoader#getSystemClassLoader() system class loader}. This method
-   * will not return null.
+   * Attempts to canonicalize null references to the system class loader.
+   * May return null if for some reason the system loader is unavailable.
    */
   private static ClassLoader canonicalize(ClassLoader classLoader) {
-    return classLoader != null
-        ? classLoader
-        : checkNotNull(getSystemClassLoaderOrNull(), "Couldn't get a ClassLoader");
+    return classLoader != null ? classLoader : SystemBridgeHolder.SYSTEM_BRIDGE.getParent();
   }
 
   /**
-   * Returns the system classloader, or {@code null} if we don't have
-   * permission.
-   */
-  private static ClassLoader getSystemClassLoaderOrNull() {
-    try {
-      return ClassLoader.getSystemClassLoader();
-    } catch (SecurityException e) {
-      return null;
-    }
-  }
-
-  /**
    * Returns the class loader to host generated classes for {@code type}.
    */
   public static ClassLoader getClassLoader(Class<?> type) {
@@ -133,23 +122,30 @@
   }
 
   private static ClassLoader getClassLoader(Class<?> type, ClassLoader delegate) {
-    delegate = canonicalize(delegate);
 
-    // if the application is running in the System classloader, assume we can run there too
-    if (delegate == getSystemClassLoaderOrNull()) {
+    // simple case: do nothing!
+    if (!CUSTOM_LOADER_ENABLED) {
       return delegate;
     }
 
-    // Don't bother bridging existing bridge classloaders
-    if (delegate instanceof BridgeClassLoader) {
+    delegate = canonicalize(delegate);
+
+    // no need for a bridge if using same class loader, or it's already a bridge
+    if (delegate == GUICE_CLASS_LOADER || delegate instanceof BridgeClassLoader) {
       return delegate;
     }
 
-    if (HOOK_ENABLED && Visibility.forType(type) == Visibility.PUBLIC) {
-      return CLASS_LOADER_CACHE.get(delegate);
+    // don't try bridging private types as it won't work
+    if (Visibility.forType(type) == Visibility.PUBLIC) {
+      if (delegate != SystemBridgeHolder.SYSTEM_BRIDGE.getParent()) {
+        // delegate guaranteed to be non-null here
+        return CLASS_LOADER_CACHE.get(delegate);
+      }
+      // delegate may or may not be null here
+      return SystemBridgeHolder.SYSTEM_BRIDGE;
     }
 
-    return delegate;
+    return delegate; // last-resort: do nothing!
   }
 
   /*if[AOP]*/
@@ -193,6 +189,7 @@
      * target class. These generated classes may be loaded by our bridge classloader.
      */
     PUBLIC {
+      @Override
       public Visibility and(Visibility that) {
         return that;
       }
@@ -205,6 +202,7 @@
      * garbage collected.
      */
     SAME_PACKAGE {
+      @Override
       public Visibility and(Visibility that) {
         return this;
       }
@@ -242,26 +240,43 @@
    */
   private static class BridgeClassLoader extends ClassLoader {
 
-    public BridgeClassLoader(ClassLoader usersClassLoader) {
+    BridgeClassLoader() {
+      // use system loader as parent
+    }
+
+    BridgeClassLoader(ClassLoader usersClassLoader) {
       super(usersClassLoader);
     }
 
     @Override protected Class<?> loadClass(String name, boolean resolve)
         throws ClassNotFoundException {
 
-      // delegate internal requests to Guice class space
+      if (name.startsWith("sun.reflect")) {
+        // these reflection classes need to be loaded from the bootstrap class loader
+        return SystemBridgeHolder.SYSTEM_BRIDGE.loadClassFromParent(name, resolve);
+      }
+
       if (name.startsWith(GUICE_INTERNAL_PACKAGE) || name.startsWith(CGLIB_PACKAGE)) {
+        if (null == GUICE_CLASS_LOADER) {
+          // use special system bridge to load classes from the bootstrap class loader
+          return SystemBridgeHolder.SYSTEM_BRIDGE.loadClassFromParent(name, resolve);
+        }
         try {
           Class<?> clazz = GUICE_CLASS_LOADER.loadClass(name);
           if (resolve) {
             resolveClass(clazz);
           }
           return clazz;
-        } catch (Exception e) {
-          // fall back to classic delegation
+        } catch (Throwable e) {
+          // fall-back to classic delegation
         }
       }
 
+      return loadClassFromParent(name, resolve);
+    }
+
+    Class<?> loadClassFromParent(String name, boolean resolve)
+      throws ClassNotFoundException {
       return super.loadClass(name, resolve);
     }
   }
Index: test/com/googlecode/guice/BytecodeGenTest.java
===================================================================
--- test/com/googlecode/guice/BytecodeGenTest.java	(revision 1142)
+++ test/com/googlecode/guice/BytecodeGenTest.java	(working copy)
@@ -190,11 +190,10 @@
       }
     }).getInstance(ProxyTest.class);
 
-    // unforunately, the expected classloader depends on which class loader loaded this test.
+    // expected outcome varies depending who loaded the proxy test
     if (ProxyTest.class.getClassLoader() == systemClassLoader) {
       assertSame(testProxy.getClass().getClassLoader(), systemClassLoader);
     } else {
-      assertNotSame(testProxy.getClass().getClassLoader(), ProxyTest.class.getClassLoader());
       assertNotSame(testProxy.getClass().getClassLoader(), systemClassLoader);
     }
   }