Eliminate proxy leak by adding pre/post to unfinished proxy check.

Author: headius

core/src/main/java/org/jruby/javasupport/Java.java

@@ -438,50 +438,71 @@ public static RubyModule getProxyClass(Ruby runtime, JavaClass javaClass) {
     }
 
     public static RubyModule getProxyClass(final Ruby runtime, final Class<?> clazz) {
-        RubyModule unfinished = runtime.getJavaSupport().getUnfinishedProxyClassCache().get(clazz).get();
-        if (unfinished != null) return unfinished;
+        RubyModule proxy = runtime.getJavaSupport().getUnfinishedProxy(clazz);
+        if (proxy != null) return proxy;
         return runtime.getJavaSupport().getProxyClassFromCache(clazz);
     }
 
     // Only used by proxy ClassValue calculator in JavaSupport
     static RubyModule createProxyClassForClass(final Ruby runtime, final Class<?> clazz) {
         final JavaSupport javaSupport = runtime.getJavaSupport();
 
-        if (clazz.isInterface()) return generateInterfaceProxy(runtime, clazz);
-
-        return generateClassProxy(runtime, clazz, javaSupport);
-    }
+        RubyModule proxy;
+        RubyClass superClass = null;
+        if (clazz.isInterface()) {
+            proxy = (RubyModule) runtime.getJavaSupport().getJavaInterfaceTemplate().dup();
+        } else {
+            if (clazz.isArray()) {
+                superClass = javaSupport.getArrayProxyClass();
+            } else if (clazz.isPrimitive()) {
+                superClass = javaSupport.getConcreteProxyClass();
+            } else if (clazz == Object.class) {
+                superClass = javaSupport.getConcreteProxyClass();
+            } else {
+                // other java proxy classes added under their superclass' java proxy
+                superClass = (RubyClass) getProxyClass(runtime, clazz.getSuperclass());
+            }
+            proxy = RubyClass.newClass(runtime, superClass);
+        }
 
-    private static RubyModule generateInterfaceProxy(final Ruby runtime, final Class javaClass) {
-        if (!javaClass.isInterface()) {
-            throw runtime.newArgumentError(javaClass.toString() + " is not an interface");
+        // ensure proxy is visible down-thread
+        javaSupport.beginProxy(clazz, proxy);
+        try {
+            if (clazz.isInterface()) {
+                generateInterfaceProxy(runtime, clazz, proxy);
+            } else {
+                generateClassProxy(runtime, clazz, (RubyClass)proxy, superClass, javaSupport);
+            }
+        } finally {
+            javaSupport.endProxy(clazz);
         }
 
-        RubyModule proxyModule = (RubyModule) runtime.getJavaSupport().getJavaInterfaceTemplate().dup();
+        return proxy;
+    }
+
+    private static void generateInterfaceProxy(final Ruby runtime, final Class javaClass, final RubyModule proxy) {
+        assert javaClass.isInterface() : "not an interface: " + javaClass;
 
         // include any interfaces we extend
         final Class<?>[] extended = javaClass.getInterfaces();
-        for ( int i = extended.length; --i >= 0; ) {
+        for (int i = extended.length; --i >= 0; ) {
             RubyModule extModule = getInterfaceModule(runtime, extended[i]);
-            proxyModule.includeModule(extModule);
+            proxy.includeModule(extModule);
         }
-        Initializer.setupProxyModule(runtime, javaClass, proxyModule);
-        addToJavaPackageModule(proxyModule);
-
-        return proxyModule;
+        Initializer.setupProxyModule(runtime, javaClass, proxy);
+        addToJavaPackageModule(proxy);
     }
 
-    private static RubyModule generateClassProxy(Ruby runtime, Class<?> clazz, JavaSupport javaSupport) {
-        RubyModule proxyClass;
+    private static void generateClassProxy(Ruby runtime, Class<?> clazz, RubyClass proxy, RubyClass superClass, JavaSupport javaSupport) {
         if (clazz.isArray()) {
-            proxyClass = createProxyClass(runtime, javaSupport.getArrayProxyClass(), clazz, true);
+            createProxyClass(runtime, proxy, clazz, true);
 
             // FIXME: Organizationally this might be nicer in a specialized class
             if ( clazz.getComponentType() == byte.class ) {
                 final Encoding ascii8bit = runtime.getEncodingService().getAscii8bitEncoding();
 
                 // All bytes can be considered raw strings and forced to particular codings if not 8bitascii
-                proxyClass.addMethod("to_s", new JavaMethodZero(proxyClass, PUBLIC) {
+                proxy.addMethod("to_s", new JavaMethodZero(proxy, PUBLIC) {
                     @Override
                     public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name) {
                         ByteList bytes = new ByteList((byte[]) ((ArrayJavaProxy) self).getObject(), ascii8bit);
@@ -491,55 +512,48 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
             }
         }
         else if ( clazz.isPrimitive() ) {
-            proxyClass = createProxyClass(runtime, javaSupport.getConcreteProxyClass(), clazz, true);
+            createProxyClass(runtime, proxy, clazz, true);
         }
         else if ( clazz == Object.class ) {
             // java.lang.Object is added at root of java proxy classes
-            proxyClass = createProxyClass(runtime, javaSupport.getConcreteProxyClass(), clazz, true);
+            createProxyClass(runtime, proxy, clazz, true);
             if (NEW_STYLE_EXTENSION) {
-                proxyClass.getMetaClass().defineAnnotatedMethods(NewStyleExtensionInherited.class);
+                proxy.getMetaClass().defineAnnotatedMethods(NewStyleExtensionInherited.class);
             } else {
-                proxyClass.getMetaClass().defineAnnotatedMethods(OldStyleExtensionInherited.class);
+                proxy.getMetaClass().defineAnnotatedMethods(OldStyleExtensionInherited.class);
             }
-            addToJavaPackageModule(proxyClass);
+            addToJavaPackageModule(proxy);
         }
         else {
-            // other java proxy classes added under their superclass' java proxy
-            RubyClass superProxyClass = (RubyClass) getProxyClass(runtime, clazz.getSuperclass());
-            proxyClass = createProxyClass(runtime, superProxyClass, clazz, false);
+            createProxyClass(runtime, proxy, clazz, false);
             // include interface modules into the proxy class
             final Class<?>[] interfaces = clazz.getInterfaces();
             for ( int i = interfaces.length; --i >= 0; ) {
-                proxyClass.includeModule(getInterfaceModule(runtime, interfaces[i]));
+                proxy.includeModule(getInterfaceModule(runtime, interfaces[i]));
             }
             if ( Modifier.isPublic(clazz.getModifiers()) ) {
-                addToJavaPackageModule(proxyClass);
+                addToJavaPackageModule(proxy);
             }
         }
 
         // JRUBY-1000, fail early when attempting to subclass a final Java class;
         // solved here by adding an exception-throwing "inherited"
         if ( Modifier.isFinal(clazz.getModifiers()) ) {
             final String clazzName = clazz.getCanonicalName();
-            proxyClass.getMetaClass().addMethod("inherited", new org.jruby.internal.runtime.methods.JavaMethod(proxyClass, PUBLIC) {
+            proxy.getMetaClass().addMethod("inherited", new org.jruby.internal.runtime.methods.JavaMethod(proxy, PUBLIC) {
                 @Override
                 public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
                     throw context.runtime.newTypeError("can not extend final Java class: " + clazzName);
                 }
             });
         }
-
-        return proxyClass;
     }
 
     private static RubyClass createProxyClass(final Ruby runtime,
-                                              final RubyClass baseType, final Class<?> javaClass, boolean invokeInherited) {
-        RubyClass proxyClass;
+                                              final RubyClass proxyClass, final Class<?> javaClass, boolean invokeInherited) {
 
-        // this needs to be split, since conditional calling #inherited doesn't fit standard ruby semantics
+        final RubyClass superClass = proxyClass.getSuperClass();
 
-        final RubyClass superClass = baseType;
-        proxyClass = RubyClass.newClass(runtime, superClass);
         proxyClass.makeMetaClass( superClass.getMetaClass() );
 
         if ( Map.class.isAssignableFrom( javaClass ) ) {
@@ -1456,6 +1470,6 @@ private static void addToJavaPackageModule(RubyModule proxyClass, JavaClass java
     @Deprecated
     private static RubyClass createProxyClass(final Ruby runtime,
                                               final RubyClass baseType, final JavaClass javaClass, boolean invokeInherited) {
-        return createProxyClass(runtime, baseType, javaClass.javaClass(), invokeInherited);
+        return createProxyClass(runtime, RubyClass.newClass(runtime, baseType), javaClass, invokeInherited);
     }
 }

core/src/main/java/org/jruby/javasupport/JavaSupport.java

@@ -38,6 +38,8 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.jruby.Ruby;
 import org.jruby.RubyClass;
@@ -67,7 +69,13 @@ public IRubyObject allocateProxy(Object javaObject, RubyClass clazz) {
 
     private final ClassValue<JavaClass> javaClassCache;
     private final ClassValue<RubyModule> proxyClassCache;
-    private final ClassValue<ThreadLocal<RubyModule>> unfinishedProxyClassCache;
+    private class UnfinishedProxy extends ReentrantLock {
+        volatile RubyModule proxy;
+        UnfinishedProxy(RubyModule proxy) {
+            this.proxy = proxy;
+        }
+    }
+    private final Map<Class, UnfinishedProxy> unfinishedProxies;
     private final ClassValue<Map<String, AssignedName>> staticAssignedNames;
     private final ClassValue<Map<String, AssignedName>> instanceAssignedNames;
 
@@ -104,19 +112,20 @@ public JavaClass computeValue(Class<?> cls) {
                 return new JavaClass(runtime, cls);
             }
         });
+
         this.proxyClassCache = ClassValue.newInstance(new ClassValueCalculator<RubyModule>() {
+            /**
+             * Because of the complexity of processing a given class and all its dependencies,
+             * we opt to synchronize this logic. Creation of all proxies goes through here,
+             * allowing us to skip some threading work downstream.
+             */
             @Override
-            public RubyModule computeValue(Class<?> cls) {
+            public synchronized RubyModule computeValue(Class<?> cls) {
                 return Java.createProxyClassForClass(runtime, cls);
             }
         });
-        this.unfinishedProxyClassCache = ClassValue.newInstance(new ClassValueCalculator<ThreadLocal<RubyModule>>() {
-            @Override
-            public ThreadLocal<RubyModule> computeValue(Class<?> cls) {
-                return new ThreadLocal<RubyModule>();
-            }
-        });
-        this.staticAssignedNames =ClassValue.newInstance(new ClassValueCalculator<Map<String, AssignedName>>() {
+
+        this.staticAssignedNames = ClassValue.newInstance(new ClassValueCalculator<Map<String, AssignedName>>() {
             @Override
             public Map<String, AssignedName> computeValue(Class<?> cls) {
                 return new HashMap<String, AssignedName>();
@@ -128,6 +137,9 @@ public Map<String, AssignedName> computeValue(Class<?> cls) {
                 return new HashMap<String, AssignedName>();
             }
         });
+
+        // Proxy creation is synchronized (see above) so a HashMap is fine for recursion detection.
+        this.unfinishedProxies = new ConcurrentHashMap<Class, UnfinishedProxy>(8, 0.75f, 1);
     }
 
     public Class loadJavaClass(String className) throws ClassNotFoundException {
@@ -327,10 +339,6 @@ public Map<Set<?>, JavaProxyClass> getJavaProxyClassCache() {
         return this.javaProxyClassCache;
     }
 
-    public ClassValue<ThreadLocal<RubyModule>> getUnfinishedProxyClassCache() {
-        return unfinishedProxyClassCache;
-    }
-
     public ClassValue<Map<String, AssignedName>> getStaticAssignedNames() {
         return staticAssignedNames;
     }
@@ -339,6 +347,23 @@ public ClassValue<Map<String, AssignedName>> getInstanceAssignedNames() {
         return instanceAssignedNames;
     }
 
+    public void beginProxy(Class cls, RubyModule proxy) {
+        UnfinishedProxy up = new UnfinishedProxy(proxy);
+        up.lock();
+        unfinishedProxies.put(cls, up);
+    }
+
+    public void endProxy(Class cls) {
+        UnfinishedProxy up = unfinishedProxies.remove(cls);
+        up.unlock();
+    }
+
+    public RubyModule getUnfinishedProxy(Class cls) {
+        UnfinishedProxy up = unfinishedProxies.get(cls);
+        if (up != null && up.isHeldByCurrentThread()) return up.proxy;
+        return null;
+    }
+
     @Deprecated
     private volatile Map<Object, Object[]> javaObjectVariables;
 

core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java

@@ -32,8 +32,6 @@ public RubyModule initialize(RubyModule proxy) {
 
         proxyClass.setReifiedClass(javaClass);
 
-        runtime.getJavaSupport().getUnfinishedProxyClassCache().get(javaClass).set(proxyClass);
-
         if ( javaClass.isArray() || javaClass.isPrimitive() ) {
             return proxy;
         }

core/src/main/java/org/jruby/javasupport/binding/Initializer.java

@@ -7,6 +7,7 @@
 import org.jruby.internal.runtime.methods.JavaMethod;
 import org.jruby.javasupport.Java;
 import org.jruby.javasupport.JavaClass;
+import org.jruby.javasupport.JavaSupport;
 import org.jruby.javasupport.JavaUtil;
 import org.jruby.runtime.Block;
 import org.jruby.runtime.Helpers;
@@ -34,6 +35,7 @@
 */
 public abstract class Initializer {
     protected final Ruby runtime;
+    protected final JavaSupport javaSupport;
     protected final Class javaClass;
 
     private static final Logger LOG = LoggerFactory.getLogger("Initializer");
@@ -68,6 +70,7 @@ public abstract class Initializer {
 
     public Initializer(Ruby runtime, Class javaClass) {
         this.runtime = runtime;
+        this.javaSupport = runtime.getJavaSupport();
         this.javaClass = javaClass;
     }
 

core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java

@@ -25,8 +25,6 @@ public RubyModule initialize(RubyModule proxy) {
 
         super.initializeBase(proxy);
 
-        runtime.getJavaSupport().getUnfinishedProxyClassCache().get(javaClass).set(proxy);
-
         Field[] fields = JavaClass.getDeclaredFields(javaClass);
 
         for (int i = fields.length; --i >= 0; ) {