Fix issues with race conditions connecting to Vault

[rmc47]

Related to #61 and possibly hashicorp/vault-ruby#216, we've been seeing a large number of Vault::ConnectionPool::PoolShuttingDownError errors running hiera_vault on a Puppet server.

There looks like there's two (related) possible race conditions in play: while a single Puppet JRuby instance serves one request at a time, I think the background thread that the Debouncer calls shutdown on can happen concurrently with the main thread's use of the Vault client.

I think this flow is possible:

• First request served; shutdown debouncer gets scheduled for T+10
• At T+9.9, second request starts being served; during execution (after configuration but before completion), debouncer fires and shuts down Vault client; logical_read operation then fails

I think there's also an issue where $vault gets set to nil following a StandardError, but not recreated when it's used on a subsequent request?

This addresses both scenarios by ensuring all use of $vault happens within a critical section. This should not substantially affect performance: the mutex should always be uncontended except where the shutdown and vault_get calls happen at the same time. It also ensures (within the critical section) that the vault client is not nil, and creates a new one if required.

GitHub's diff is pretty unhelpful due to indentation changes, I'm afraid - ignoring whitespace changes gives something much more readable. I've changed the names of the global variables to avoid any conflicts with other global variables from older versions of this module running concurrently in other Puppet environments, as I think Ruby globals are not isolated between environments.

(Caveat: my Ruby knowledge is pretty poor - apologies if I've taken the wrong approach on this.)

[petems]

This looks like a good approach, I'll see if I can quickly test it and merge this week.

Can you rebase as well? Just merged a fix to ENV setup

[rmc47]

For sure - rebased on 4376028. (I've kept it as two commits - one for the critical section, one for the variable rename to avoid environment escape - but shout if you'd prefer it as one.)

[petems]

@rmc47 So sorry, I completely forgot about this PR. I've just merged in some logic for comma-based pathing that has a bunch of refacotring, are you ok to rebase again? If not no worries, I think I get the jist of the logic here so I think I can do it.

[petems]

Ok, I think I've rebased it correctly, gonna see if I can manually check this in an acceptance test environment 👍🏻

[rmc47]

Amazing, thank you! To be honest, we'd entirely forgotten about it as well and have been running off my branch for the last couple of months 😆. Would be great to get back on to main if you're happy with the fix!

[petems]

Glad to hear! Are you ok to test this particular rebased branch as well to confirm it works? If so, I'll merge and cut a new 2.0.0 version of the module as a potential breaking change, then you can move back to the master branch 👍🏻

[rmc47]

@petems that's been running for the last few days, and all looks good our side!

[petems]

Awesome, lets get this merged and a new release cut! 😄