[Buildbot-devel] Re: Locking stacktrace

[Brian Warner]

> Actually by reading the logs I can clearly see that one is locking one
> copy of the SlaveLock, and the other is locking a different SlaveLock.
>  I guess that's the disadvantage of using instances, huh? :^(

Ack. Yeah, you're right. I checked my notes, it seems I ran into a similar
situation during testing, but I couldn't reproduce it so I figured it was
just a transient problem.

I guess the first step would be to create a reliable test case, one which
loads an initial config file, then loads a second config file (that defines
the additional Lock). The test should then look inside the resulting Builders
and make sure the lock instances are identical.

I *thought* I checked for this sort of thing, but obviously there's a hole
somewhere in that code.

> For now I'll have to put the locks (and schedulers, come to think of
> it) into another file and import it so that it is only loaded once.

Schedulers should be OK. FYI, the overall way that the config file is read is
like this:

 read and evaluate the config file, creating temporary instances of everything
 there (Schedulers, Status targets, Locks, etc)

 for each key in the config dict, compare the existing instances against the
 temporary ones (using ==). Any instances that aren't already in the existing
 set get added, any existing instances that aren't in the temporary set get
 removed.

 config file reload is complete, all temporary instances are released

The ComparableMixin parent class implements __cmp__ to make sure that two
Scheduler instances with the same parameters evaluate as ==, so reloading the
config file without any changes will not cause the existing Schedulers to be
replaced. In twisted.application.service terms, the new (and redundant)
instances are instantiated, but not started (they are never given a parent,
and their startService() method is never invoked).

So reloading the config file when you haven't actually changed anything
should leave everything alone.

But, now that I look at the code, there is no such main "existing instances"
list of Locks, so the instance that gets used depends entirely upon whether
the container object (a Builder, for example) was itself changed or not. So
yeah, you could wind up with different instances of the same Lock, which is
obviously broken.

Hrm.. I'm not sure what the best fix would be. Going to string identifiers
instead of actual Lock instances does indeed sound promising, but I'm leaning
towards using instances in general instead of strings, so I'm hoping to find
a different solution.

pondering,
 -Brian