More of mcculls fixes for choosing which classloader to use for generated classes. I believe this solves the package-private problem and the memory leak problem. Nice work Stuart!
git-svn-id: https://google-guice.googlecode.com/svn/trunk@544 d779f126-a31b-0410-b53b-1d3aecad763e
diff --git a/src/com/google/inject/DefaultConstructionProxyFactory.java b/src/com/google/inject/DefaultConstructionProxyFactory.java
index ace1f93..b508431 100644
--- a/src/com/google/inject/DefaultConstructionProxyFactory.java
+++ b/src/com/google/inject/DefaultConstructionProxyFactory.java
@@ -16,6 +16,7 @@
package com.google.inject;
+import com.google.inject.internal.BytecodeGen.Visibility;
import static com.google.inject.internal.BytecodeGen.newFastClass;
import com.google.inject.internal.Errors;
import com.google.inject.internal.ErrorsException;
@@ -39,9 +40,8 @@
throws ErrorsException {
final List<Parameter<?>> parameters = Parameter.forConstructor(constructor, errors);
- // We can't use FastConstructor if the constructor is private or protected.
- if (Modifier.isPrivate(constructor.getModifiers())
- || Modifier.isProtected(constructor.getModifiers())) {
+ // We can't use FastConstructor if the constructor is non-public.
+ if (!Modifier.isPublic(constructor.getModifiers())) {
constructor.setAccessible(true);
return new ConstructionProxy<T>() {
public T newInstance(Object... arguments) throws
@@ -67,7 +67,7 @@
return new ConstructionProxy<T>() {
Class<T> classToConstruct = constructor.getDeclaringClass();
- FastClass fastClass = newFastClass(classToConstruct);
+ FastClass fastClass = newFastClass(classToConstruct, Visibility.PUBLIC);
final FastConstructor fastConstructor = fastClass.getConstructor(constructor);
@SuppressWarnings("unchecked")
diff --git a/src/com/google/inject/InjectorImpl.java b/src/com/google/inject/InjectorImpl.java
index d337d85..4b3c282 100644
--- a/src/com/google/inject/InjectorImpl.java
+++ b/src/com/google/inject/InjectorImpl.java
@@ -23,6 +23,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Sets;
+import com.google.inject.internal.BytecodeGen.Visibility;
import static com.google.inject.internal.BytecodeGen.newFastClass;
import com.google.inject.internal.Classes;
import com.google.inject.internal.Errors;
@@ -884,7 +885,8 @@
}
};
} else {
- FastClass fastClass = newFastClass(method.getDeclaringClass());
+ FastClass fastClass = newFastClass(method.getDeclaringClass(),
+ Visibility.forMember(method));
final FastMethod fastMethod = fastClass.getMethod(method);
methodInvoker = new MethodInvoker() {
diff --git a/src/com/google/inject/ProxyFactory.java b/src/com/google/inject/ProxyFactory.java
index 13bb646..f43cc9f 100644
--- a/src/com/google/inject/ProxyFactory.java
+++ b/src/com/google/inject/ProxyFactory.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
+import com.google.inject.internal.BytecodeGen.Visibility;
import static com.google.inject.internal.BytecodeGen.newEnhancer;
import static com.google.inject.internal.BytecodeGen.newFastClass;
import com.google.inject.internal.Errors;
@@ -114,11 +115,16 @@
indices.put(method, i);
}
+ // true if all the methods we're intercepting are public. This impacts which classloader we
+ // should use for loading the enhanced class
+ Visibility visibility = Visibility.PUBLIC;
+
// Iterate over aspects and add interceptors for the methods they apply to
boolean anyMatched = false;
for (MethodAspect methodAspect : applicableAspects) {
for (MethodInterceptorsPair pair : methodInterceptorsPairs) {
if (methodAspect.matches(pair.method)) {
+ visibility = visibility.and(Visibility.forMember(pair.method));
pair.addAll(methodAspect.interceptors());
anyMatched = true;
}
@@ -147,7 +153,7 @@
}
// Create the proxied class.
- Enhancer enhancer = newEnhancer(declaringClass);
+ Enhancer enhancer = newEnhancer(declaringClass, visibility);
enhancer.setCallbackFilter(new CallbackFilter() {
public int accept(Method method) {
return indices.get(method);
@@ -168,7 +174,7 @@
*/
private <T> ConstructionProxy<T> createConstructionProxy(Errors errors, final Class<?> clazz,
final Constructor standardConstructor) throws ErrorsException {
- FastClass fastClass = newFastClass(clazz);
+ FastClass fastClass = newFastClass(clazz, Visibility.PUBLIC);
final FastConstructor fastConstructor
= fastClass.getConstructor(standardConstructor.getParameterTypes());
final List<Parameter<?>> parameters = Parameter.forConstructor(standardConstructor, errors);
diff --git a/src/com/google/inject/internal/BytecodeGen.java b/src/com/google/inject/internal/BytecodeGen.java
index 35f5458..019a8d0 100644
--- a/src/com/google/inject/internal/BytecodeGen.java
+++ b/src/com/google/inject/internal/BytecodeGen.java
@@ -17,10 +17,11 @@
package com.google.inject.internal;
import static com.google.inject.internal.ReferenceType.WEAK;
-import static java.lang.reflect.Modifier.PROTECTED;
-import static java.lang.reflect.Modifier.PUBLIC;
+import java.lang.reflect.Member;
+import java.lang.reflect.Modifier;
import java.security.AccessController;
import java.security.PrivilegedAction;
+import java.util.logging.Logger;
import net.sf.cglib.core.DefaultNamingPolicy;
import net.sf.cglib.core.NamingPolicy;
import net.sf.cglib.proxy.Enhancer;
@@ -57,6 +58,8 @@
*/
public final class BytecodeGen {
+ private static final Logger logger = Logger.getLogger(BytecodeGen.class.getName());
+
static final ClassLoader GUICE_CLASS_LOADER = BytecodeGen.class.getClassLoader();
/** ie. "com.google.inject.internal" */
@@ -74,9 +77,8 @@
};
/** Use "-Dguice.custom.loader=false" to disable custom classloading. */
- // disabled until assisted inject tests pass
static final boolean HOOK_ENABLED
- = "true".equals(System.getProperty("guice.custom.loader", "false"));
+ = "true".equals(System.getProperty("guice.custom.loader", "true"));
/**
* Weak cache of bridge class loaders that make the Guice implementation
@@ -85,6 +87,12 @@
private static final ReferenceCache<ClassLoader, ClassLoader> CLASS_LOADER_CACHE
= new ReferenceCache<ClassLoader, ClassLoader>(WEAK, WEAK) {
@Override protected ClassLoader create(final ClassLoader typeClassLoader) {
+ // Don't bother bridging existing bridge classloaders
+ if (typeClassLoader instanceof BridgeClassLoader) {
+ return typeClassLoader;
+ }
+
+ logger.fine("Creating a bridge ClassLoader for " + typeClassLoader);
return AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() {
public ClassLoader run() {
return new BridgeClassLoader(typeClassLoader);
@@ -94,16 +102,6 @@
};
/**
- * Returns true if {@code type} can be loaded from our custom class loader.
- * Private/implementation classes are not visible to classes loaded by other
- * class loaders, even when they are in the same package. Therefore we cannot
- * intercept classloading requests for such types.
- */
- private static boolean isHookable(Class<?> type) {
- return (type.getModifiers() & (PROTECTED | PUBLIC)) != 0;
- }
-
- /**
* For class loaders, {@code null}, is always an alias to the
* {@link ClassLoader#getSystemClassLoader() system class loader}.
*/
@@ -123,32 +121,84 @@
private static ClassLoader getClassLoader(Class<?> type, ClassLoader delegate) {
delegate = canonicalize(delegate);
- if (HOOK_ENABLED && isHookable(type)) {
+ if (HOOK_ENABLED && Visibility.forType(type) == Visibility.PUBLIC) {
return CLASS_LOADER_CACHE.get(delegate);
}
return delegate;
}
- public static FastClass newFastClass(Class<?> type) {
+ public static FastClass newFastClass(Class<?> type, Visibility visibility) {
FastClass.Generator generator = new FastClass.Generator();
generator.setType(type);
- generator.setClassLoader(getClassLoader(type));
+ if (visibility == Visibility.PUBLIC) {
+ generator.setClassLoader(getClassLoader(type));
+ }
generator.setNamingPolicy(NAMING_POLICY);
+ logger.fine("Loading " + type + " FastClass with " + generator.getClassLoader());
return generator.create();
}
- public static Enhancer newEnhancer(Class<?> type) {
+ public static Enhancer newEnhancer(Class<?> type, Visibility visibility) {
Enhancer enhancer = new Enhancer();
enhancer.setSuperclass(type);
enhancer.setUseCache(false); // We do enough caching.
enhancer.setUseFactory(false);
+ if (visibility == Visibility.PUBLIC) {
+ enhancer.setClassLoader(getClassLoader(type));
+ }
enhancer.setNamingPolicy(NAMING_POLICY);
- enhancer.setClassLoader(getClassLoader(type));
+ logger.fine("Loading " + type + " Enhancer with " + enhancer.getClassLoader());
return enhancer;
}
/**
+ * The required visibility of a user's class from a Guice-generated class. Visibility of
+ * package-private members depends on the loading classloader: only if two classes were loaded by
+ * the same classloader can they see each other's package-private members. We need to be careful
+ * when choosing which classloader to use for generated classes. We prefer our bridge classloader,
+ * since it's OSGi-safe and doesn't leak permgen space. But often we cannot due to visibility.
+ */
+ public enum Visibility {
+
+ /**
+ * Indicates that Guice-generated classes only need to call and override public members of the
+ * target class. These generated classes may be loaded by our bridge classloader.
+ */
+ PUBLIC {
+ public Visibility and(Visibility that) {
+ return that;
+ }
+ },
+
+ /**
+ * Indicates that Guice-generated classes need to call or override package-private members.
+ * These generated classes must be loaded in the same classloader as the target class. They
+ * won't work with OSGi, and won't get garbage collected until the target class' classloader is
+ * garbage collected.
+ */
+ SAME_PACKAGE {
+ public Visibility and(Visibility that) {
+ return this;
+ }
+ };
+
+ public static Visibility forMember(Member member) {
+ return (member.getModifiers() & (Modifier.PROTECTED | Modifier.PUBLIC)) != 0
+ ? PUBLIC
+ : SAME_PACKAGE;
+ }
+
+ public static Visibility forType(Class<?> type) {
+ return (type.getModifiers() & (Modifier.PROTECTED | Modifier.PUBLIC)) != 0
+ ? PUBLIC
+ : SAME_PACKAGE;
+ }
+
+ public abstract Visibility and(Visibility that);
+ }
+
+ /**
* Loader for Guice-generated classes. For referenced classes, this delegates to either either the
* user's classloader (which is the parent of this classloader) or Guice's class loader.
*/
