Construction of DefaultEnvironment can be racy

[mwichmann]

This captures a discussion from #4424

The global function DefaultEnvironment is a factory function that returns the default environment object (effectively a singleton) which is a special environment instance. As an additional quirk, the factory then replaces itself with another function which just immediately returns the object, so once the singleton is fully initialized, it is very fast and very safe. However, the actual creation can potentially be quite slow:

def DefaultEnvironment(*args, **kwargs):
    """Construct the global ("default") construction environment.

    The environment is provisioned with the values from *kwargs*.

    After the environment is created, this function is replaced with
    a reference to :func:`_fetch_DefaultEnvironment` which efficiently
    returns the initialized default construction environment without
    checking for its existence.

    Historically, some parts of the code held references to this function.
    Thus it still has the existence check for :data:`_default_env` rather
    than just blindly creating the environment and overwriting itself.
    """
    global _default_env
    if not _default_env:
        _default_env = SCons.Environment.Environment(*args, **kwargs)
        _default_env.Decider('content')
        global DefaultEnvironment
        DefaultEnvironment = _fetch_DefaultEnvironment
        _default_env._CacheDir_path = None
    return _default_env

If nothing particular has been said in a project's SConscripts, the call to Environment() (which will be assigned to _default_env) needs not only to create and initialize the instance, but go through the whole tool setup process, which can include calls to external commands to ask them about versions and/or setup information. Thus, it is possible that in a multithreaded build, a second event could require the DefaultEnvironment, call the factory, end up in the initial version because the overwrite has not happened yet, see _default_env is not yet set because the first instance is still working, and proceed to try to create another instance.

Any problems can be mitigated by explicitly calling DefaultEnvironment(tools=[]) (which many SCons tests do, for performance reasons), which greatly reduces the timing window, and also forces the creation to happen early and not in the multithreaded build phase. However, asking users to put in a dummy call for purposes that are not visible (i.e. "help avoid an internal error") seems like an unreasonable burden.

[bdbaddog]

I guess I don't see how this can happen. Aren't all calls to DefaultEnvironment() in the Sconstruct/SConscripts?
And thus not run in parallel with each other.

They's not in the DAG walk/build right?

[mwichmann]

Calls can be triggered internally, when code that wants a bit of context finds it doesn't have enough (should that ever happen?), although I haven't searched in detail what implications all of these have. For example, the CommandGeneratorAction class can do this in an odd place. Things related to CacheDir can do this (this was the case that triggered the initial report). There's someplace in Node as well.

[mwichmann]

In particular, it seems this is a likely culprit - a method of any environment:

    def get_CacheDir(self):
        try:
            path = self._CacheDir_path
        except AttributeError:
            path = SCons.Defaults.DefaultEnvironment()._CacheDir_path

The referenced PR proposed wrapping the DefaultEnvironment call here in an additional try block, but this is a place where you'd love to see the tool setup be lazy-evaluated instead.

Would actually prefer not to go down the path of instantiating the default environment here at all, but I guess that's the place the value of a bare CacheDir call will have been stored - but in that case, the default environment will have to have been already created in order for it to be stored there, right? So I'm more confused than before.

[bdbaddog]

Yes. This is very odd. Maybe we can get the #4424 author to instrument his scons to dump stack traces of DefaultEnvironment() calls ?