Thread conflict in Puppet::Pops::Loaders

[puppetlabs]

Thread conflict in Puppet::Pops::Loaders

[joshcooper]

This was originally reported in https://puppet.atlassian.net/browse/PUP-11324. That issue tracker is now read-only, so please continue discussion here.

[joshcooper]

Thanks for letting us know @jstange We currently use the environment object to protect against concurrent access, such as

puppet/lib/puppet/pops/loaders.rb

Line 28 in b04e822

environment.lock.synchronize do

I think you'd want to do something similar for each of the Loaders instance methods that mutate state, like:

 def [](loader_name)
   environment.lock.synchronize do
      loader = @loaders_by_name[loader_name]
      if loader.nil?
        # Unable to find the module private loader. Try resolving the module
        loader = private_loader_for_module(loader_name[0..-9]) if loader_name.end_with?(' private')
        raise Puppet::ParseError, _("Unable to find loader named '%{loader_name}'") % { loader_name: loader_name } if loader.nil?
    end
    loader
end

Alternatively you could only lock when loader.nil?, but I think you need to relookup again inside the lock (double-checked lock)

def [](loader_name)
  loader = @loaders_by_name[loader_name]
  if loader.nil?
    environment.lock.synchronize do
     if loader.nil?
       ...

We don't have any immediate plans for working on this, but we welcome pull requests. I'll put the help wanted label on for now.

[jstange]

After some poking I might have some vague idea how threading works in here. Seems like it's JRuby-specific, and in the case I care about it's probably instances of Puppet::Util::Autoload that are competing to touch things in parallel.

[] probably isn't the only method that needs a thread seatbelt put on it, but I don't want to blindly add stuff and create potential deadlock conditions.

I'll spend some time with my stack traces and see if there's anywhere else that looks thread-dangerous and deadlock-safe, wrap those in environment.lock.synchronize, and submit a PR of it. Someone who knows what they're doing will have to review- my guesswork about threading in here may be wildly off base.

[github-actions]

Migrated issue to PUP-12038

[jstange]

Finally got my corporate overlords to sign off on the CLA for the #9299. Our fix has been deployed locally for a while now and seems to solve the issue.

We've recently identified a similar, but distinct, threading issue, this one in Resource::Type. I'll open a separate issue for it once I've made sure my report isn't duplicating someone else's.