On Jun 14, 2010, at 10:53 AM, Markus Roberts wrote:
On Sat, Jun 12, 2010 at 9:40 PM, Luke Kanies <[email protected]>
wrote:
On Jun 11, 2010, at 4:10 PM, Markus Roberts wrote:
Jesse and I are shooting for the minimal viable fix here, with the
idea that
a great deal of refactoring is needed but isn't appropriate at this
time. The
changes in this commit are:
* Index the function-holding modules by environment
* We need to know the "current environment" when we're defining a
function so
we can attach it to the proper module, and this information isn't
dynamically
available when user-defined functions are being created (we're being
called by
user written code that doesn't "know" about environments) so we
cheat and
stash the value in Puppet::Node::Environment
Is there any attempt at thread-safety here? I couldn't see any,
which kind of concerns me.
No. This is not thread safe, which is not a change; none of the
code involved was thread safe and this patch does not make it so.
As discussed in person - the current (released) code is thread safe
because it has no shared state, but this code introduces shared state
with no safety for managing it.
Don't we only need that value set during loading? The rest of the
time the env value should be available. That is, functions should
only be loaded within the context of an environment, so 'current'
shouldn't be necessary then, right?
The earlier version of the patch was written that way, and that
refactor should ultimately be done, but it became increasingly
complicated the deeper we got into it and (since the quasi-global
Environment.current was needed in any case) we decided to accept it
as technical debt, use it as the single (and far simpler) mechanism
and move on for now.
Again as discussed in person, in nearly every case where the 'default'
env is used, it's probably a bug.
I also don't see how this code knows when a given function should be
loaded into the root module vs. the current env?
* since we must do this anyway, it turns out to be cleaner & safer
to do the
same when we are evaluating a functon. This is the main change from
the prior
version of this patch.
I missed this code somehow... What do you mean?
That we decided that having a single mechanism (Environment.current)
was cleaner & would be easier to eventually unwind than having two.
This is basically the point I just mentioned above.
So did you remove every instance of 'Environment.new' with no
arguments? That would seem to be the natural consequence. And if so,
where did those callers get their default environments?
* Add a special *root* environment for the built in functions, and
extend all
scopes with it.
* Index the function characteristics (name, type, docstring, etc.)
by environment
* Make the autoloader environment aware, so that it uses the
modulepath for the
specified environment rather than the default
* Turn off caching of the modulepath since it potentially changes
for each node
Note that this is potentially expensive - the module search path is
built from the lib dir of all modules in that env, so building this
path involves looking in every module in the env. Removing the
cache can hurt. It might be a better idea to cache per environment.
If it turns out to be a significant performance hit, certainly.
This was done as the result of user experience, and it's worth at
least checking to see how many real-life stats get called when we
don't cache this any more.
As with your patch, this was a step along the path from "not working
at all" to "working really well"; it gets us to "working, though
perhaps not elegant or performant" but Jesse and I are by no means
in love with the state that it leaves us in.
-- Markus
P.S. Personally, I think stripping out all pretense of handling
multiple environments in a single process is the way to go, but I
understand the concerns that make that an unpopular direction.
Tell it to the users. :/
--
If all the world's a stage, I want to operate the trap door.
-- Paul Beatty
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199
--
You received this message because you are subscribed to the Google Groups "Puppet
Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
