Environ config changes: to lock or not to lock?

[roger peppe]

On 24 June 2013 10:47, Jeroen Vermeulen <jeroen.vermeulen at canonical.com> wrote:
> Hi all,
>
> The provider implementations contain mutexes that protect attributes
> against concurrent config changes.  But I still feel a little unsure
> about exactly what we can assume, and I was hoping you could give me
> some pointers to answers, or hints that we can turn into documentation:
>
> Do we want consistent images of an environ and its config, so that a
> provider operation concurrent with a config change doesn't get a view
> that's nonsensical in some way?  Or are the locks just there to prevent
> undefined behaviour at the language level?

The locks are there principally for the latter, yes, because in general
we allow methods to be called concurrently on an Environ
and allowing concurrent access to fields invokes undefined behaviour.

> We're mostly looking at the ec2 provider as an example, but it's hard to
> get this stuff right by imitation: why is it safe for providers to set
> environ.name, when read access to that attribute is not lock-protected?

That is actually a bug; that's a good catch. There is not, AFAICS any
good reason to have a name field in the environ.  Alternatively, we
could keep the name field and set it only in the Open method. There's a
guarantee that the environment name cannot be changed (see bug 1018207)
so there's no need to change it in general when SetConfig is called.

I've proposed a fix: https://codereview.appspot.com/10492043

>  When we release an environment's lock between reading multiple
> attributes, is there a risk of reconfiguration happening inbetween, and
> do we mind?  It'd be great to have the reasons explicit in the code.

In the most general case, there is such a risk, but we take care not to
share an Environ between concurrent workers, so any individual worker
can decide whether it is appropriate to call a method or function which
may call SetConfig (it's free to use a mutex itself if it needs to).

There are a few more such places than there used to be
(environs.FindBootstrapTools and environs.EnsureCertificate being two),
so it's definitely worth being aware of this potential issue.

That said, it's easy to read multiple attributes without worrying about
the locking - just use the result of Environ.Config, which is valid
forever (although it may hold stale information).  That at least makes
it easy to ensure that all the attributes are consistent with one another.

I can't currently think of a situation in which using slightly stale
attributes may be a problem (attributes usually change in response to
a change in the environment configuration in the state, which observers
will see after an arbitrary time delay anyway).

Some additional documentation on environs.Environ.SetConfig may
be appropriate.

  cheers,
    rog.