From 5259c6812ba59e00a0cdb084f759206c25e99b4f Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Wed, 15 Feb 2012 11:16:21 -0600 Subject: [PATCH] Fix JRUBY-6456 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 --- src/org/jruby/Ruby.java | 58 ++++++++++++++++++++++++++++++++++-- src/org/jruby/RubyArray.java | 49 ++++++++++++++++++------------ 2 files changed, 86 insertions(+), 21 deletions(-) diff --git a/src/org/jruby/Ruby.java b/src/org/jruby/Ruby.java index 254c115e27f..abfbbc84aed 100644 --- a/src/org/jruby/Ruby.java +++ b/src/org/jruby/Ruby.java @@ -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; @@ -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. @@ -3532,6 +3534,17 @@ public void unregisterInspecting(Object obj) { if (val != null ) val.remove(obj); } + public T recursiveListOperation(Callable 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); } @@ -3567,6 +3580,13 @@ private IRubyObject recursiveListAccess() { return list; } + private void recursiveListClear() { + Map hash = recursive.get(); + if(hash != null) { + hash.clear(); + } + } + private RubySymbol recursiveKey; private static class ExecRecursiveParams { @@ -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); } diff --git a/src/org/jruby/RubyArray.java b/src/org/jruby/RubyArray.java index 6945ae842c3..97bef50f6b5 100644 --- a/src/org/jruby/RubyArray.java +++ b/src/org/jruby/RubyArray.java @@ -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; @@ -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() { + 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 @@ -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()); @@ -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"; @@ -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); @@ -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() { + public IRubyObject call() { + return joinAny(context, RubyArray.this, sepStringFinal, iFinal, joinStrings(sepStringFinal, iFinal, result)); + } + }); } len += ((RubyString) tmp).getByteList().length();