Skip to content

Commit

Permalink
Eliminate proxy leak by adding pre/post to unfinished proxy check.
Browse files Browse the repository at this point in the history
  • Loading branch information
@headius
headius committed Apr 24, 2015
1 parent 4752304 commit 2bdfb5a
Showing 5 changed files with 94 additions and 56 deletions.
92 changes: 53 additions & 39 deletions core/src/main/java/org/jruby/javasupport/Java.java
View file
Original file line number Diff line number Diff line change
@@ -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);
}
}
51 changes: 38 additions & 13 deletions core/src/main/java/org/jruby/javasupport/JavaSupport.java
View file
Original file line number Diff line number Diff line change
@@ -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;

2 changes: 0 additions & 2 deletions core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java
View file
Original file line number Diff line number Diff line change
@@ -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;
}
3 changes: 3 additions & 0 deletions core/src/main/java/org/jruby/javasupport/binding/Initializer.java
View file
Original file line number Diff line number Diff line change
@@ -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;
}

2 changes: 0 additions & 2 deletions core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java
View file
Original file line number Diff line number Diff line change
@@ -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; ) {

1 comment on commit 2bdfb5a

@kares
Copy link
Member

@kares kares commented on 2bdfb5a Apr 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like that there's no longer a thread-local and the synchronized trick to avoid threads competing initialization.
also more readable + initializer no longer need to worry about setting the proxy ... definitely a 👍 from me!

one thing to (possibly) double-check I recall the inherited callback was a little "sensitive" to refactorings - hopefully there are some tests for that.

Please sign in to comment.