Issue #8442

[iuri-gg]

So I tried using hub gem to do a pull request for the following issue: #8442

But I keep getting 422 so I decided to stop fighting it and do it through github's GUI.If someone can attach the pull request to issue 8442 that would be awesome.

Thanks.

[carlosantoniodasilva]

/cc @fxn

[fxn]

I am concentrating spare time on the contrib app this week, will be back shortly.

[iuri-gg]

to make clear. this test passes on master branch but will fail on 3-2 stable (since thats the place were leak is and its not present in 4.0)

[senny]

@fxn can you take another look at this one?

@igagnidz to use hub to attach pull-requests you need to have commit access to the repository ;)

[fxn]

I don't see code related to singleton classes in the implementation. If AS::Dependecies is not defined, everything is cleared, if it does, I believe singleton classes would remain because autolodaded? returns false for anonymous modules.

Can you elaborate a bit more please?

[iuri-gg]

@fxn First post in #8442 describes the memory leak issue. You are right that autoloaded? returns false for singleton classes and therefore it is not cleared from AS::DescendantsTracker which results in memory leak (and which I claim is a problem for dynamic applications - it leaked 1GB ram after 1K requests for my app). Test that is attached to this pull request demonstrates that behaviour in 3.2 branch (4.0 branch has AS::DescendantsTracker completely rewritten and test doesn't break).

[fxn]

Yep, I see the potential leak.

It is interesting that it doesn't fail in master, from reading the source code I'd say the leak is there, see nothing to prevent it... so either the test environment is the one that has changed and dependencies is not defined (so we clear everything), or else I miss something reading the source code. I want to understand it either way :).

I'll play with it a little bit.

[thedarkone]

@fxn: the master doesn't leak because of mine thread-safety related DescendantsTracker refactoring. The change you're looking for is when @@direct_descendants is ever changed.

In the new code it is only during the Class#inherited callback (which is not invoked whenever a singleton is created) whereas in the old code this happens whenever direct_descendants method is called for the first time (because of the @@direct_descendants's default Hash.new { |h, k| h[k] = [] } block). So, in the old code singleton.direct_descendants stores the singleton (as a hash key) in the @@direct_descendants whereas the new code does no such thing.

Solution: backport 9f84e60 and a3ad0a7 into 3-2-stable.

[fxn]

@thedarkone but if you autoload a class, create an instance, force the creation of the singleton class of the instance as a key in the tracker as in the test... who is going to clean that singleton class?

Imagine the autoloaded class is gone, the instance is gone... the singleton class won't be cleared and won't be garbage collected. But the next request running that code will create a new singleton class.

Am I wrong? That's the leak I see. I need to experiment with the code, only wondering from reading the code so perhaps it is not working that way and I am missing something.

[fxn]

Ahhh, but the test is not autoloading. So if the leak I described exists, it is a different leak (and statistically with less impact I guess).

[thedarkone]

parent_instance = Parent.new
singleton_class = parent_instance.singleton_class

# now call .descendants
singleton_class.descendants

In the old code the problem occurs here: @@direct_descendants[klass] looks up a singleton klass in the @@direct_descendants, but the key (klass) doesn't exist yet so the Hash's default block gets triggered and does: @@direct_descendants[klass] = [], which causes a leak.

In the new code calling Class#descendants goes through DescendantsTracker's #descendants and #accumulate_descendants - both of these methods only read from @@direct_descendants (which no longer has any default block, as that would not be thread safe) and singleton_class never gets stored in @@direct_descendants.

[fxn]

Hey, I am at the computer.

Thanks for the explanation, I understand the patch better now. I need then to illustrate in a different way the potential leak I see:

Class.new(Class.new(User))

That not only leaks the grandchild, but also the superclasses via the superclass pointers. The three of them leak.

While reading the source code the suspicious detail that caught my attention was that anonymous classes are ignored altogether. There are a number of ways to get them, though they may not be common in regular code.

I believe that if a class has been autoloaded, all its descendants should be cleared. Singleton or not singleton, anonymous or not. Otherwise you leak upwards. I believe the singleton class is not leaking in master due to a side-effect of the refactor, but the core issue has not been addressed.

Nevertheless, let's go for the patch in this pull request. I believe it needs some little editing before being accepted. I would remove the comments because it is no longer true that the singleton class is added. Would be enough to write a comment above the method definition saying that this is a regression test for #8442 (with its link). Also would use key? rather than keys.include?.