(PUP-10942) Don't leak environment state when creating a new instance #8523
Conversation
|
👍 This seems like a good approach |
joshcooper
force-pushed
the
envconf_caching_10942
branch
from
c046281 to
552f127
Compare
Our `include_in_any_order` matcher predates `contain_exactly`.
joshcooper
force-pushed
the
envconf_caching_10942
branch
from
552f127 to
54b23f4
Compare
Environment state for an expired environment leaked into newly created instances, if the cached loader's `list` or `get_conf` methods had been called prior to expiration. This doesn't occur if `get` is called, because the environment is cached, and the per-environment settings are cleared when `clear` or `clear_all` are called. Clear per-environments settings prior to creating the environment instance.
joshcooper
force-pushed
the
envconf_caching_10942
branch
2 times, most recently
from
83a56a3 to
a8b4e6c
Compare
When using versioned code deploys, `list` and `get` could return different modulepaths for the same environment between the time new code was deployed and when the environment was expired. Now the "list" method returns the cached environment (if there is one) or the newly created environment object. In the latter case, we cache the environment, so it can be cleared if/when clear or clear_all are called for that environment.
joshcooper
force-pushed
the
envconf_caching_10942
branch
from
a8b4e6c to
8d569e8
Compare
| @@ -18,7 +25,7 @@ def self.an_executable(path) | |||
| def self.a_directory(path, children = []) | |||
| new(path, | |||
| :exist? => true, | |||
| :excutable? => true, | |||
| :executable? => true, | |||
|
|
||
| @loader.list.map do |env| | ||
| name = env.name | ||
| old_entry = @cache[name] |
There was a problem hiding this comment.
there's nothing here we need to worry about regarding symbols vs strings?
There was a problem hiding this comment.
I walked through it and I guess strings vs symbols just worries me in general. But I don't see where it would have an effect.
There was a problem hiding this comment.
I kept the existing behavior of using whatever name is passed to get. Here we check if the name is cached and if not, we pass the name to add_entry and it stores it under that key. Similarly, if we're asked to clear a specific entry, we just use that name.
I agree the string vs symbol thing could bite us, so I filed https://tickets.puppetlabs.com/browse/PUP-10955
|
jenkins please test this with servertests |
Environment state for an expired environment leaked into newly created instances, if the cached loader's
listorget_confmethods had been called prior to expiration. This doesn't occur ifgetis called, because the environment is cached, and the per-environment settings are cleared whenclearorclear_allare called.When using versioned code deploys,
listandgetcould return different modulepaths for the same environment between the time new code was deployed and when the environment was expired. Now the "list" method returns the cached environment (if there is one) or the newly created environment object. In the latter case, we cache the environment, so it can be cleared if/when clear or clear_all are called for that environment.