Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PUP-2692) Fix memory leak in directory environments #2714

Conversation

@zaphod42
Copy link
Contributor

zaphod42 commented May 30, 2014

The directory environments exhibited a memory leak when the environment_timeout
!= unlimited. The problem came down to equality of Puppet::Node::Environment
being incorrect and the fact that the directory environments refresh caches by
recreating instances of that class.

zaphod42 added 2 commits May 30, 2014
The Puppet::Parser::Functions code used to hold onto two different sets
of information about the loaded functions: a hash of environment name to
environment module, and a hash of *environment* to a hash of function
name to function information. It was assumed that the
Puppet::Node::Environment class was written to properly work as a hash
key since it implemented both hash() and ==(). This assumption being
true would have meant that different instances for the same environment
would be the same key and the second hash above would not grow beyond a
reasonable size for the number of environments.

It turns out that to be a proper hash key a class must implement hash()
and eql?() (not ==()). Because Puppet::Node::Environment didn't do this
the hash of function information would just continually grow when
directory environments with an environment_timeout != unlimited was in
use. This is because the Puppet::Node::Environment instance would be
periodically thrown out by the Puppet::Environments::Cached loader and
be recreated, but the Puppet::Parser::Functions hash would hold onto the
old instance.

The fix for this here is to consolidate all of the function information
into a single structure. The function modules now have a few methods to
get the function metadata instead of it being stored separately. This is
completely backwards compatible and does not conflict with any existing
uses of the environment modules since the new methods are on the module
itself and will not be part of the mixin.
This fixes environment equality to make it work as a hash key. It also
adds tests around this functionality.
@puppetcla

This comment has been minimized.

Copy link

puppetcla commented May 30, 2014

CLA signed by all contributors.

@hlindberg

This comment has been minimized.

Copy link
Contributor

hlindberg commented May 31, 2014

What clears the Functions' bindings of defined functions. Isn't this still leaking, only less so?

@zaphod42

This comment has been minimized.

Copy link
Contributor Author

zaphod42 commented Jun 2, 2014

@hlindberg Yeah, there is technically a leak in the hash of function modules. I've thought about moving the function module for an environment onto the environment instance itself, but that might cause other problems because of reloading code when the Puppet::Node::Environment is recreated.

The slow leak that exists right now in the functions is mostly dealt with by the fact that passenger workers recycle periodically, thereby cleaning up the hash.

hlindberg added a commit that referenced this pull request Jun 2, 2014
…eak-in-master

(PUP-2692) Fix memory leak in directory environments
@hlindberg hlindberg merged commit b6a2561 into puppetlabs:stable Jun 2, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.