Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Recursive checking threadlocals in Ruby keep runtimes alive #162

Closed
headius opened this Issue · 5 comments

2 participants

@headius
Owner

We have a couple thread locals in Ruby that are used for recursive calls like Array#inspect. Unfortunately, because they reference Ruby objects, they end up keeping their associated runtime alive for the life of the thread.

We need to either not use thread locals or not use Ruby objects as values.

@bbrowning

Patch provided by headius in https://gist.github.com/2639787 helps the issue but I can't yet confirm that it completely fixes things.

@headius headius referenced this issue from a commit
@headius headius Partial solution for #162.
This clears the ThreadLocal objects out, so they can GC, but it
causes the referenced objects to linger because ThreadLocal's weak
referencing still requires a few GC cycles. The better fix will be
to not use Ruby objects in the thread-locals, but that requires a
reimplementation of the recursion logic.
a4edae5
@bbrowning

I'm starting to wonder why recursiveKey is a ThreadLocal at all? It was made a ThreadLocal in 46c59ad but I think just making inRecursiveListOperation a ThreadLocal from that commit is enough to fix the issue that commit fixed. I tried putting recursiveKey back to just a simple RubySymbol and the recursive_check_thread_safety_spec still passes.

The only solution that's allowed TorqueBox integs to pass under 1.9 mode in JRuby 1.7 without OOMing is with the recursiveKey a regular RubySymbol instead of a ThreadLocal.

@headius
Owner

They're thread-local because otherwise two threads doing recursive operations at the same time would step on each other. They might not need to be quite as thread-local as they are now, though.

@bbrowning

I think it's enough for inRecursiveListOperation to be ThreadLocal and not recursiveKey. Since recursiveKey is a read-only RubySymbol with a value that gets set during runtime initialization I don't see how another thread could step on its value.

@bbrowning

I believe the patch at https://gist.github.com/2696701 fixes this issue but I'm having a hard time verifying since MemoryAnalyzer and jhat keep choking on my 1.4GB heap dump. A heap dumped from TorqueBox integration tests running under a recent build of JRuby 1.7 with the mentioned patch applied is at http://dl.dropbox.com/u/9213410/torquebox_jruby17.bin if you'd like to take a look. There are still quite a few org.jruby.Ruby instances around but I've yet to find one in that dump with a path to GC root of this recursiveKey.

@headius headius closed this issue from a commit
@headius headius Fix #162
Ruby.recursiveKey is never set, so it doesn't need to be a
ThreadLocal. Thanks to Ben Browning for the fix.
7ca1995
@headius headius closed this in 7ca1995
@jsvd jsvd referenced this issue from a commit in jsvd/jruby
@headius headius Use thread-local state for all recursive checks.
Backport from 46c59ad. Partial fix for #162.
3aab61b
@jsvd jsvd referenced this issue from a commit in jsvd/jruby
@headius headius Partial solution for #162.
This clears the ThreadLocal objects out, so they can GC, but it
causes the referenced objects to linger because ThreadLocal's weak
referencing still requires a few GC cycles. The better fix will be
to not use Ruby objects in the thread-locals, but that requires a
reimplementation of the recursion logic.
268fdc4
@jsvd jsvd referenced this issue from a commit in jsvd/jruby
@headius headius Fix #162
Ruby.recursiveKey is never set, so it doesn't need to be a
ThreadLocal. Thanks to Ben Browning for the fix.
ca7ff04
@headius headius referenced this issue from a commit
@headius headius Use thread-local state for all recursive checks.
Backport from 46c59ad. Partial fix for #162.
2f73450
@headius headius referenced this issue from a commit
@headius headius Partial solution for #162.
This clears the ThreadLocal objects out, so they can GC, but it
causes the referenced objects to linger because ThreadLocal's weak
referencing still requires a few GC cycles. The better fix will be
to not use Ruby objects in the thread-locals, but that requires a
reimplementation of the recursion logic.
d96b69e
@headius headius referenced this issue from a commit
@headius headius Fix #162
Ruby.recursiveKey is never set, so it doesn't need to be a
ThreadLocal. Thanks to Ben Browning for the fix.
1d1ec53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.