Skip to content

Commit

Permalink
Fix JRUBY-6456
Browse files Browse the repository at this point in the history 
ThreadLocal Leak in 1.9 Mode w/ Internal Recursive Map

Our code was a direct port of MRI's, and they seem to have the
same issue: no logic clears out the threadlocal map once the
recursive walk is complete. I added an outermost method called
recursiveListOperation that should always be used to wrap the
execRecursive* calls.

Conflicts:

	src/org/jruby/Ruby.java
  • Loading branch information
@headius
headius committed Feb 15, 2012
1 parent d642c26 commit 5259c68
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 21 deletions.
58 changes: 56 additions & 2 deletions src/org/jruby/Ruby.java
View file
Expand Up @@ -57,6 +57,7 @@
import java.util.Stack;
import java.util.Vector;
import java.util.WeakHashMap;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.SynchronousQueue;
import java.util.concurrent.ThreadPoolExecutor;
Expand Down Expand Up @@ -145,6 +146,7 @@
import org.jruby.runtime.load.BasicLibraryService;
import org.jruby.threading.DaemonThreadFactory;
import org.jruby.util.io.SelectorPool;
import org.jruby.util.unsafe.UnsafeFactory;

/**
* The Ruby object represents the top-level of a JRuby "instance" in a given VM.
Expand Down Expand Up @@ -3532,6 +3534,17 @@ public void unregisterInspecting(Object obj) {
if (val != null ) val.remove(obj);
}

public <T extends IRubyObject> T recursiveListOperation(Callable<T> body) {
try {
return body.call();
} catch (Exception e) {
UnsafeFactory.getUnsafe().throwException(e);
return null; // not reached
} finally {
recursiveListClear();
}
}

public static interface RecursiveFunction {
IRubyObject call(IRubyObject obj, boolean recur);
}
Expand Down Expand Up @@ -3567,6 +3580,13 @@ private IRubyObject recursiveListAccess() {
return list;
}

private void recursiveListClear() {
Map<String, RubyHash> hash = recursive.get();
if(hash != null) {
hash.clear();
}
}

private RubySymbol recursiveKey;

private static class ExecRecursiveParams {
Expand Down Expand Up @@ -3684,12 +3704,46 @@ private IRubyObject execRecursiveInternal(RecursiveFunction func, IRubyObject ob
}
}

// rb_exec_recursive
/**
* Perform a recursive walk on the given object using the given function.
*
* Do not call this method directly unless you know you're within a call
* to {@link Ruby#recursiveListOperation(java.util.concurrent.Callable) recursiveListOperation},
* which will ensure the thread-local recursion tracking data structs are
* cleared.
*
* MRI: rb_exec_recursive
*
* Calls func(obj, arg, recursive), where recursive is non-zero if the
* current method is called recursively on obj
*
* @param func
* @param obj
* @return
*/
public IRubyObject execRecursive(RecursiveFunction func, IRubyObject obj) {
return execRecursiveInternal(func, obj, null, false);
}

// rb_exec_recursive_outer
/**
* Perform a recursive walk on the given object using the given function.
* Treat this as the outermost call.
*
* Do not call this method directly unless you know you're within a call
* to {@link Ruby#recursiveListOperation(java.util.concurrent.Callable) recursiveListOperation},
* which will ensure the thread-local recursion tracking data structs are
* cleared.
*
* MRI: rb_exec_recursive_outer
*
* If recursion is detected on the current method and obj, the outermost
* func will be called with (obj, arg, Qtrue). All inner func will be
* short-circuited using throw.
*
* @param func
* @param obj
* @return
*/
public IRubyObject execRecursiveOuter(RecursiveFunction func, IRubyObject obj) {
return execRecursiveInternal(func, obj, null, true);
}
Expand Down
49 changes: 30 additions & 19 deletions src/org/jruby/RubyArray.java
View file
Expand Up @@ -50,6 +50,7 @@
import java.util.List;
import java.util.ListIterator;
import java.util.RandomAccess;
import java.util.concurrent.Callable;

import org.jcodings.Encoding;
import org.jcodings.specific.USASCIIEncoding;
Expand Down Expand Up @@ -705,22 +706,26 @@ public RubyFixnum hash(ThreadContext context) {
*/
@JRubyMethod(name = "hash", compat = RUBY1_9)
public RubyFixnum hash19(final ThreadContext context) {
return (RubyFixnum)getRuntime().execRecursiveOuter(new Ruby.RecursiveFunction() {
public IRubyObject call(IRubyObject obj, boolean recur) {
int begin = RubyArray.this.begin;
long h = realLength;
if(recur) {
h ^= RubyNumeric.num2long(invokedynamic(context, context.runtime.getArray(), HASH));
} else {
for(int i = begin; i < begin + realLength; i++) {
h = (h << 1) | (h < 0 ? 1 : 0);
final IRubyObject value = safeArrayRef(values, i);
h ^= RubyNumeric.num2long(invokedynamic(context, value, HASH));
return context.runtime.recursiveListOperation(new Callable<RubyFixnum>() {
public RubyFixnum call() {
return (RubyFixnum) getRuntime().execRecursiveOuter(new Ruby.RecursiveFunction() {
public IRubyObject call(IRubyObject obj, boolean recur) {
int begin = RubyArray.this.begin;
long h = realLength;
if (recur) {
h ^= RubyNumeric.num2long(invokedynamic(context, context.runtime.getArray(), HASH));
} else {
for (int i = begin; i < begin + realLength; i++) {
h = (h << 1) | (h < 0 ? 1 : 0);
final IRubyObject value = safeArrayRef(values, i);
h ^= RubyNumeric.num2long(invokedynamic(context, value, HASH));
}
}
return getRuntime().newFixnum(h);
}
return getRuntime().newFixnum(h);
}
}, this);
}, RubyArray.this);
}
});
}

/** rb_ary_store
Expand Down Expand Up @@ -1772,7 +1777,7 @@ public IRubyObject join(ThreadContext context) {
return join(context, context.getRuntime().getGlobalVariables().get("$,"));
}

// 1.9 MRI: join0
// 1.9 MRI: ary_join_0
private RubyString joinStrings(RubyString sep, int max, RubyString result) {
if (max > 0) result.setEncoding(values[begin].convertToString().getEncoding());

Expand All @@ -1788,7 +1793,7 @@ private RubyString joinStrings(RubyString sep, int max, RubyString result) {
return result;
}

// 1.9 MRI: join1
// 1.9 MRI: ary_join_1
private RubyString joinAny(ThreadContext context, IRubyObject obj, RubyString sep,
int i, RubyString result) {
assert i >= begin : "joining elements before beginning of array";
Expand Down Expand Up @@ -1846,7 +1851,7 @@ public IRubyObject call(IRubyObject obj, boolean recur) {
*
*/
@JRubyMethod(name = "join", compat = RUBY1_9)
public IRubyObject join19(ThreadContext context, IRubyObject sep) {
public IRubyObject join19(final ThreadContext context, final IRubyObject sep) {
final Ruby runtime = context.getRuntime();

if (realLength == 0) return RubyString.newEmptyString(runtime, USASCIIEncoding.INSTANCE);
Expand All @@ -1863,9 +1868,15 @@ public IRubyObject join19(ThreadContext context, IRubyObject sep) {
IRubyObject tmp = val.checkStringType19();
if (tmp.isNil() || tmp != val) {
len += ((begin + realLength) - i) * 10;
RubyString result = (RubyString) RubyString.newStringLight(runtime, len, USASCIIEncoding.INSTANCE).infectBy(this);
final RubyString result = (RubyString) RubyString.newStringLight(runtime, len, USASCIIEncoding.INSTANCE).infectBy(this);
final RubyString sepStringFinal = sepString;
final int iFinal = i;

return joinAny(context, this, sepString, i, joinStrings(sepString, i, result));
return runtime.recursiveListOperation(new Callable<IRubyObject>() {
public IRubyObject call() {
return joinAny(context, RubyArray.this, sepStringFinal, iFinal, joinStrings(sepStringFinal, iFinal, result));
}
});
}

len += ((RubyString) tmp).getByteList().length();
Expand Down

0 comments on commit 5259c68

Please sign in to comment.