Bug #6030: Thread-local "leak" in rb_exec_recursive*

• Status: Closed
• Assignee: headius (Charles Nutter)
• Target version: -
• ruby -v: 1.9.3 head
• Backport: 2.4: UNKNOWN, 2.5: UNKNOWN

[headius (Charles Nutter)]

I believe there may be a "leak" in the rb_exec_recursive* functions in thread.c.

We have ported these methods for recursion detection in JRuby, and as in MRI they use a map inside a thread-local to track method name + object references. However, none of these method ever clean up the thread local when the recursive walk is complete.

The thread-local data is initialized in thread.c, recursive_list_access, around line 3819 (in 1.9.3 branch):

if (NIL_P(hash) || TYPE(hash) != T_HASH) {
hash = rb_hash_new();
OBJ_UNTRUST(hash);
rb_thread_local_aset(rb_thread_current(), recursive_key, hash);

As far as I can tell, this thread-local is never cleared, holding a hash reference for as long as the thread is alive.

[headius (Charles Nutter)]

I forgot to point out a couple things...

• By "leak" I mean this will hold references to data longer than it should. It's not a true leak in that it is bounded.

• It is bounded, but only by method names; recursive_list_access keys those thread-locals based on the current method name from the current frame, which means it will "leak" as many hash objects as there are unique methods utilizing rb_exec_recursive*.

From recursive_list_access:

...
VALUE sym = ID2SYM(rb_frame_this_func());
...
list = rb_hash_new();
OBJ_UNTRUST(list);
rb_hash_aset(hash, sym, list);

In JRuby, I fixed this by having rb_exec_recursive_outer clean up the thread-local upon completion, and I added recursive_list_operation (Ruby.java, recursiveListOperation) as a new "outermost" function to wrap our version of rb_exec_recursive. our rb_exec_recursive also gained an safeguard + error if it is called without being inside recursive_list_operation.

The commits are here:

https://github.com/jruby/jruby/commit/8f2ba4214eb070a83951aef1f143ebf6ebcecce4

https://github.com/jruby/jruby/commit/f7c1ffd93f34cef816168f36f5fe4f960a9ac2e2

It was a more severe problem for JRuby because the hash (a RubyHash) held references to the current JRuby instance, keeping it alive for as long as the thread was alive. Still a "leak", but a larger amount of bounded data held for too long.

[marcandre (Marc-Andre Lafortune)]

Hi.

I'm not sure I see where there is a problem.

If Array#==, say, is called in a given thread, it's quite likely that it will be called again later on, so why not keep the reference to the empty hash ready for next call?

As long as these hashes are empty at the end of the rb_exec_recursive_*, it's not clear that there is much memory to recoup. It could also affect the performance.

[headius (Charles Nutter)]

It would be possible to bloat up memory use by calling many method names that all hit this logic; saving a hash per name and never releasing it seems unnecessary.

If it's not a big enough deal for MRI to fix, so be it...it was an issue in JRuby because of the additional rooting of JRuby instances, but JRuby support MVM.

For that matter, this also is highly unlikely to work with the MVM stuff, though I know that's still off in experimental-land anyway.

It just seems like a good idea to clean up after the recursive logic is done, rather than leaving a dirty hash lying around.

[ko1 (Koichi Sasada)]

• Assignee set to nobu (Nobuyoshi Nakada)
• Priority changed from Normal to 3

[shyouhei (Shyouhei Urabe)]

• Status changed from Open to Assigned

[mame (Yusuke Endoh)]

• Status changed from Assigned to Feedback
• Assignee changed from nobu (Nobuyoshi Nakada) to headius (Charles Nutter)

This ticket was discussed at today's developer meeting.

Nobu said that per-method entries are removed when it becomes unneeded. So, the "leak" is one hash object per thread. Correct? If so, it is not a big deal. Let us know if we missed something.

[jeremyevans0 (Jeremy Evans)]

• Status changed from Feedback to Closed