Jira (PUP-10659) Data Type casts cause Puppet Server to retain Compiler instances

Charlie Sharpsteen (Jira)

unread,

Sep 4, 2020, 12:10:03 PM9/4/20

to puppe...@googlegroups.com

Charlie Sharpsteen updated an issue

Puppet /

PUP-10659

Data Type casts cause Puppet Server to retain Compiler instances

Change By:	Charlie Sharpsteen
Summary:	Data Type casts case cause Puppet Server to retain Compiler instances

Add Comment

This message was sent by Atlassian Jira (v8.5.2#805002-sha1:a66f935)

Charlie Sharpsteen (Jira)

unread,

Sep 4, 2020, 12:51:03 PM9/4/20

to puppe...@googlegroups.com

Charlie Sharpsteen commented on

PUP-10659

Re: Data Type casts cause Puppet Server to retain Compiler instances

The cause of this appears to be two behaviors in the following section of code in lib/puppet/functions/new.rb:

def new_function_for_type(t, scope)

  @new_function_cache ||= Hash.new() {|hsh, key| hsh[key] = key.new_function.new(scope, loader) }

  @new_function_cache[t]

end

https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/functions/new.rb

This maintains a cache of constructor functions for each datatype: new String, new Integer, etc. The two issues are:

The cache is a Hash instance with a lambda function that creates constructor functions as they are requested. This lambda closes over the Compiler scope value passed the first time new() is called, which keeps that scope in memory.

The Compiler scope value passed the first time new() is called is further passed into the constructor function when it is created. The function then stores it as a local variable: https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/pops/functions/function.rb#L23

Aside from the memory leak, it seems wrong to be using the scope of the first Compiler that happens to call new() to provide data for subsequent compilations.

Add Comment

Charlie Sharpsteen (Jira)

unread,

Sep 4, 2020, 1:37:04 PM9/4/20

to puppe...@googlegroups.com

Charlie Sharpsteen commented on

PUP-10659

Re: Data Type casts cause Puppet Server to retain Compiler instances

The following re-factor of the new_function_for method appears to solve the leak:

def new_function_for_type(t, scope)

  @new_function_cache ||= {}

  if ! @new_function_cache.key?(t)

    @new_function_cache[t] = t.new_function.new(nil, loader)

end

  @new_function_cache[t]

end

The main functional change is passing nil when creating the constructor function instead of passing scope. It seems like this should be fine as functions will fall back to using the current Compiler's global scope if the value previously populated by scope is referenced:

https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/pops/functions/function.rb#L78-L79

Also, having constructor functions created in subsequent compilations be dependent on the scope of the first Compiler ever created seems like it would be bug.

Add Comment

Charlie Sharpsteen (Jira)

unread,

Sep 4, 2020, 3:16:04 PM9/4/20

to puppe...@googlegroups.com

Charlie Sharpsteen commented on

PUP-10659

Re: Data Type casts cause Puppet Server to retain Compiler instances

Initializing the constructor functions with the current compilers scope seems to echo ~~PUP-6582~~. This commit in Puppet changed most function initialization to use nil as the closure scope:

https://github.com/puppetlabs/puppet/commit/915d934ad06

Unless new() is special in some way, it seems like the same rationale would apply.