Merge lp:~vila/bzr/config-lock-remote into lp:bzr

[Vincent Ladeuil]

Support config remote branch config file.

This add supports for the remote branch.conf files.

It's an incremental improvement, compatible with all servers since it relies on the vfs layer only.

I plan to add a smart verb only when I switch to the new lock scheme (single read/write for a given config file for a given bzr operation).

[Martin Pool (mbp)]

On 26 May 2011 16:33, Vincent Ladeuil <email address hidden> wrote:
> @@ -2372,13 +2384,13 @@
>         # part of an object (roughly a Stack, directly or indirectly) that is
>         # an attribute of the branch object itself. Since the BranchStore
>         # cannot exist without a branch, it's safe to make it a weakref.
> -        self.branch = weakref.ref(branch)
> +        self.branch_ref = weakref.ref(branch)
>
>     def lock_write(self, token=None):
> -        return self.branch.lock_write(token)
> +        return self.branch_ref().lock_write(token)
>
>     def unlock(self):
> -        return self.branch.unlock()
> +        return self.branch_ref().unlock()
>
>     @needs_write_lock
>     def save(self):

How does that work?

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/26/2011 08:33 AM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/config-lock-remote into lp:bzr with lp:~vila/bzr/config-lock-branch as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/config-lock-remote/+merge/62418
>
> This add supports for the remote branch.conf files.
>
> It's an incremental improvement, compatible with all servers since it relies on the vfs layer only.
>
> I plan to add a smart verb only when I switch to the new lock scheme (single read/write for a given config file for a given bzr operation).
>
>
     def lock_write(self, token=None):
- - return self.branch.lock_write(token)
+ return self.branch_ref().lock_write(token)
...

     def unlock(self):
- - return self.branch.unlock()
+ return self.branch_ref().unlock()

^- If you are using a weak_ref you need to handle when branch_ref()
returns None. (because the branch has gone away)

I realize this shouldn't generally happen (I assume the branch is
holding a direct reference to the config object), but it is something
you should at least be prepared for.

Otherwise this just looks like some minor tweaking to the existing code,
which seems fine to me.

John
=:->

 review: approve

(Pending responses to Martin's questions.)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3d/cYACgkQJdeBCYSNAAPkhACgoTg1RLzRI/rdvsy5WZgksqJx
FLAAoJPXZA6UrekhZYDI6dNMHWwTOaxD
=a0ub
-----END PGP SIGNATURE-----

review: Approve

[Vincent Ladeuil (vila)]

> How does that work?

Pretty well :)

Jokes aside, it works *now* because the tests are holding a reference to the branch object while the config object is alive.

Once we start making this object an attribute of the branch, we'll also get the guarantee that the branch is always alive while we use the config object.

Note this pattern is already used in our code base in SmartClientHTTPMedium and _DebugCounter.

[Vincent Ladeuil (vila)]

> ^- If you are using a weak_ref you need to handle when branch_ref()
> returns None. (because the branch has gone away)

Having encountered the issue while implementing the associated tests, I know it's handled: an exception is raised making it obvious that the branch is dead. Since this reveals a bug in the code I don't plan to explicitly test for it (since the gc is involved it's inherently racy).

> I realize this shouldn't generally happen (I assume the branch is
> holding a direct reference to the config object),

Not yet, but that's the plan yes.

> but it is something
> you should at least be prepared for.

See above, I didn't have to think twice to understand what was happening, really.

> review: approve

Thanks, the pre-requisite branches are still pending though.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/26/2011 09:41 AM, Vincent Ladeuil wrote:
>> ^- If you are using a weak_ref you need to handle when branch_ref()
>> returns None. (because the branch has gone away)
>
> Having encountered the issue while implementing the associated tests, I know it's handled: an exception is raised making it obvious that the branch is dead. Since this reveals a bug in the code I don't plan to explicitly test for it (since the gc is involved it's inherently racy).
>
>> I realize this shouldn't generally happen (I assume the branch is
>> holding a direct reference to the config object),
>
> Not yet, but that's the plan yes.
>
>> but it is something
>> you should at least be prepared for.
>
> See above, I didn't have to think twice to understand what was happening, really.
>
>> review: approve
>
> Thanks, the pre-requisite branches are still pending though.
>

Getting at AttributeError is an ugly way to say the branch has
disappeared. Very hard to discover when it happens in production
(accidentally certainly, but still possible).

So please consider giving us a nicer error for discoverability purposes.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eCTMACgkQJdeBCYSNAAN8RwCgvB9OuL2oOJahq4hr966qw8PH
su0An10Hfy8GOpvtJNOOiIXw3k5KFYQr
=Bqhx
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel writes:

<snip/>

    > Getting at AttributeError is an ugly way to say the branch has
    > disappeared. Very hard to discover when it happens in production
    > (accidentally certainly, but still possible).

    > So please consider giving us a nicer error for discoverability purposes.

Could you point me where in the *actual* code this idiom is used and
such a reporting is taken into account ?

I find it highly unproductive to be blocked from landing incremental
patches when my intent is to ease review.

This code is *not* used in production right now and when it will this
case will never be triggered (and could not possibly be). The only cases
now and tomorrow are unit tests which purposefully *avoid* creating such
objects *without* a protecting branch. And these tests already have a
comment explaining the issue.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/26/2011 12:10 PM, Vincent Ladeuil wrote:
>>>>>> John Arbash Meinel writes:
>
> <snip/>
>
> > Getting at AttributeError is an ugly way to say the branch has
> > disappeared. Very hard to discover when it happens in production
> > (accidentally certainly, but still possible).
>
> > So please consider giving us a nicer error for discoverability purposes.
>
> Could you point me where in the *actual* code this idiom is used and
> such a reporting is taken into account ?
>
> I find it highly unproductive to be blocked from landing incremental
> patches when my intent is to ease review.
>
> This code is *not* used in production right now and when it will this
> case will never be triggered (and could not possibly be). The only cases
> now and tomorrow are unit tests which purposefully *avoid* creating such
> objects *without* a protecting branch. And these tests already have a
> comment explaining the issue.
>

We don't use weakrefs in the code very much, and where a None can be an
easily accidental return from something where we expect to get an object.

So there isn't a lot of prior art, but I *have* seen a fair number of
AttributeError bugs filed that are hard to discover.

I think using weakref without being careful about its side-effects is
poor form. Even if it "can't happen". Bugs happen.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eJ74ACgkQJdeBCYSNAAOXHACgnRpcgesoDZv1xssVgF3lO+mb
oVQAnRXVszFF95iGWJ/zW0yRc616Y4GH
=1tkW
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> Could you point me where in the *actual* code this idiom is used and
> such a reporting is taken into account ?
>
> I find it highly unproductive to be blocked from landing incremental
> patches when my intent is to ease review.

I should also mention that I'm not blocking. Specifically I said:

> John
> =:->
>
> review: approve
>
And then later I said:
> So please consider giving us a nicer error for discoverability purposes.
            ^^^^^^^^

Both are meant as, "I really think this would be better if it trapped
for None, but if you feel strongly you can land it".

Bugs happen, in places we don't want them to. Getting AttributeError is
a really confusing way of finding out that a branch disappeared when we
expected it to exist. If you used a normal reference the code would be
clearer and would *actually* never fail. I believe you are doing this
because of working recently with Martin gz and running into issues with
python cycles taking a while to be closed by the garbage collector.

ATM, I feel that is premature optimization (Branch has about 5 different
cycles, so unless we close them all, it isn't really worth making this
code harder to follow on the off chance that we clean other stuff up.)

I'm willing to live with it, provided we don't make things harder later
because of hard-to-track-down-reference-counting bugs and getting
AttributeError: None has no attribute unlock().

It is obvious to you *now*, will it be obvious in 2 years when we get a
bug report because we cleaned up something that used to have a cycle and
as a side effect kept the branch alive and people start expecting that
behavior.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eKTQACgkQJdeBCYSNAAPJbwCghXd6lUrK0JF5cVTkG9P76nmm
dg0An0KtgA3EcUla0BnkUI5deZjBlyp+
=FzKW
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel writes:

<snip/>

    > We don't use weakrefs in the code very much,
    > and where a None can be an easily accidental return from something
    > where we expect to get an object.

Not, it's not easy, it has to be deliberate. The branch object used here
is a *mandatory* parameter of the constructor.

Creating such an object and keeping it while *deleting* the branch is
not only outside of the obvious use case, it's deliberate sabotage and
as I already mentioned, you already get a traceback.

    > So there isn't a lot of prior art, but I *have* seen a fair number
    > of AttributeError bugs filed that are hard to discover.

    > I think using weakref without being careful about its side-effects is
    > poor form.

Which is why I *am* careful. There is *one* usage so far in a test with
a comment. There will be a *single* other one once this is deployed.

    > Even if it "can't happen". Bugs happen.

You still not answer how you want it to be reported. At the point where
it can occur you only have a traceback at best which is already what
will be reported.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...
> > I think using weakref without being careful about its side-effects is
> > poor form.
>
> Which is why I *am* careful. There is *one* usage so far in a test with
> a comment. There will be a *single* other one once this is deployed.
>
> > Even if it "can't happen". Bugs happen.
>
> You still not answer how you want it to be reported. At the point where
> it can occur you only have a traceback at best which is already what
> will be reported.
>

branch = self.branch_ref()
if branch is None:
  raise RuntimeError('Our branch disappeared, but the branch should'
       ' be holding a direct reference to this config'
       ' object. %(s)' % (self,)

Something like that would be much easier to understand if it happens
than just AttributeError('None has no attribute unlock')

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eLbwACgkQJdeBCYSNAANGTwCeKPoIs8YjY5aUDyEpwD5XP9gu
ueoAnjyCvEqwqZH8lzcBU7RApJM3FmGn
=d4Jf
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>
    > And then later I said:
    >> So please consider giving us a nicer error for discoverability purposes.
    > ^^^^^^^^

Ok, sorry for the knee-jerk reaction.

    > Both are meant as, "I really think this would be better if it trapped
    > for None,

But trapped to report what ?

We already get a traceback in this case and we can't get more than that.

    > but if you feel strongly you can land it".

But I can't land since two pre-requisite are still waiting (and two
already approved branches in addition to this one still can't land
either).

    > Bugs happen, in places we don't want them to. Getting
    > AttributeError is a really confusing way of finding out that a
    > branch disappeared when we expected it to exist.

When this code is deployed the config attribute won't be able to exist
without its containing branch, it's not as if it was intended to live in
weird places.

    > If you used a normal reference the code would be clearer

And the cycle well hidden and *that* would be far more harder to track
down.

If you were to encounter a traceback mentioning one of the weafref lines,
the annotations will give you the context where it has been introduced
which itself will show you the tests explaining the rationale.

And this in a class which mentions:

# FIXME: Moreover, we shouldn't need classes for these stores either, factory
# functions or a registry will make it easier and clearer for tests, focusing
# on the relevant parts of the API that needs testing -- vila 20110503 (based
# on a poolie's remark)

So we're still talking about scaffolding stuff here...

    > and would *actually* never fail.

Oh right, there will be no bugs to dereference the ref, but the bugs
implied by the cycle are the kind I'd really prefer to avoid.

    > I believe you are doing this because of working recently with
    > Martin gz and running into issues with python cycles taking a
    > while to be closed by the garbage collector.

I'm always paying attention to cycles, some are more obvious than
others. Using closures for example, as gz was trying to avoid, is one
important use case I want to keep and that I discussed with him at
length and where, in tests, we could easily address by using addCleanups
*without* stopping to use closures (see my comments on his first
proposal months ago). Yet, closures doesn't create cycles that the gc
can't deal with which is why I had troubles understanding gz approach
(which requires detecting cycles on objects that are about to be
collectible but not fully at the point he want to check).

Here, the cycle *is* obvious. SmartClientHTTPMedium is also an obvious
case and the pattern is exactly the same: an object keeps a reference to
a parent object which, by design, will live longer than itself.

I was explicitly violating this in the *test* when I encounter the issue
which I why I put the comment there (3 times no less).

Adding some reporting into SmartClientHTTPMedium sounds as weird as
adding it there.

    > ATM, I feel that is premature optimization (Branch has about 5
    > different cycles, so unless we close them all, it i...

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    > branch = self.branch_ref()
    > if branch is None:
    > raise RuntimeError('Our branch disappeared, but the branch should'
    > ' be holding a direct reference to this config'
    > ' object. %(s)' % (self,)

    > Something like that would be much easier to understand if it happens
    > than just AttributeError('None has no attribute unlock')

Done, fixing typos, it won't make a big difference, you still need the
backtrace anyway and when it occurs (unlikely but whatever) the bug will
most probably be in code not mentioned there.

[Martin Packman (gz)]

From reading the discussion here, it seems there's not really any disagreement on the core points, just a few implementation specifics. The change to raise an assertion looks good to.

Of interest to this topic is bug 758652 where John found there were lots of Branch objects being kept alive leading to loggerhead memory problems. There are two approaches to fixing that, one is to avoid cyclic data structures, such as by using weakref here. The other is a finalize method that neuters the branch, which would include things like manually deleting attributes that formed cycles. While a finalize method doesn't exist, avoiding adding more objects that reference each other is sensible.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/26/2011 06:31 PM, Martin [gz] wrote:
>>From reading the discussion here, it seems there's not really any disagreement on the core points, just a few implementation specifics. The change to raise an assertion looks good to.
>
> Of interest to this topic is bug 758652 where John found there were lots of Branch objects being kept alive leading to loggerhead memory problems. There are two approaches to fixing that, one is to avoid cyclic data structures, such as by using weakref here. The other is a finalize method that neuters the branch, which would include things like manually deleting attributes that formed cycles. While a finalize method doesn't exist, avoiding adding more objects that reference each other is sensible.

I think finalize is more sensible. Branch specifically is just hard to
prevent from cycles, because even stuff like dynamic dispatch causes a
ref-cycle.

For example:

class SimpleSelfRef(object)

  def __init__(self):
    self.actions = {'foo': self.do_foo, 'bar': self.do_bar}

  def do_foo(self):
    return 'foo'

  def do_bar(self):
    return 'bar'

  def do_action(self, action):
    return self.actions[action]()

The 'self.actions' dict forms a cycle. I'm not sure where we have that,
but I'm sure we did at one point. And I think Branch hands off
references of functions to Repository, but it has been a long time since
I've looked at it.

It is very natural to write something like:

 self.helper = Helper(self.take_action)

And all of that stuff creates cycles. And as long as helper does need
some state from 'self', it is going to have a cycle. The best you could
do is pass self explicitly as a weakref and take_action as a non-bound
method. But it really makes the code harder rather than just .finalize().

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3erDsACgkQJdeBCYSNAAOC5ACfT29avv/iTnqYItWkgt6Evl5G
qpgAoMN8dNS0z71O8JaUHrs5XVWy5QXe
=gVKi
-----END PGP SIGNATURE-----

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
[…]
> But trapped to report what ?
>
> We already get a traceback in this case and we can't get more than that.

Tracebacks alone do tell you which line of code broke, but:

 1) not all bug reports have tracebacks readily available. e.g. the
    smart server doesn't send tracebacks to the client, but does send
    the str of the exception.
 2) not all bug reports have the exact bzr version included, which makes
    matching up line numbers hard. Especially from installs without .py
    files, so tracebacks only include file names, line numbers and
    function names.
 3) Code evolves, and what produces an unambiguous traceback today might
    not in bzr 3.1.

Perhaps none of those is going to be an issue here, but I'm unconvinced.
I think John's suggestion to report “weakref of branch disappeared too
early” is clearly more informative than a generic AttributeError, and
not hard to do. If I were very confident we'd never see this error I
wouldn't care, but this (using weakrefs to avoid cycles) is a new idiom
for us and I think it's reasonable to expect we'll make a few mistakes
with it.

[Martin Pool (mbp)]

Like John said recently, the bar for review is that things be better
than they are, not that they be perfect. But generally, making sure
that forseeable errors give a sensible message seems to have a very
good payoff.

[Martin Pool (mbp)]

The .unload docstring doesn't really make it clear to me who would be supposed to call it, or when. Perhaps it should be in the developer guide for configuration?

36 + # We don't want to create a cycle here when the BranchStore becomes
37 + # part of an object (roughly a Stack, directly or indirectly) that is
38 + # an attribute of the branch object itself. Since the BranchStore
39 + # cannot exist without a branch, it's safe to make it a weakref.
40 + self.branch_ref = weakref.ref(branch)

Why do cycles matter here?

145 + raise AssertionError('Specify both ``transport_class`` and '
146 + '``server_class`` or none of them')

To be pedantic, you say "neither" if there are two, and "none" if there are more.

review: Needs Information

[Vincent Ladeuil (vila)]

> The .unload docstring doesn't really make it clear to me who would be supposed
> to call it, or when.

I introduced unload as the abstract way to express self._config_obj = None to trigger a reload from the store when needed.

This is required to match the actual behaviour which requires re-reading the whole config file before modifying a single option value.

It will probably be used in the future implementation but only once when writing the Store.

> Perhaps it should be in the developer guide for configuration?

Until I use unload() in the future implementation I'm unsure about whether it will still be needed :-/

>
> 36 + # We don't want to create a cycle here when the BranchStore
> becomes
> 37 + # part of an object (roughly a Stack, directly or indirectly)
> that is
> 38 + # an attribute of the branch object itself. Since the
> BranchStore
> 39 + # cannot exist without a branch, it's safe to make it a
> weakref.
> 40 + self.branch_ref = weakref.ref(branch)
>
> Why do cycles matter here?

Err, I should be missing the point of the question. Assuming you've read the comments above, you know we already have cycles involving branches so I'd rather not introduce a new one when I can avoid it.

>
> 145 + raise AssertionError('Specify both ``transport_class`` and '
> 146 + '``server_class`` or none of them')
>
> To be pedantic, you say "neither" if there are two, and "none" if there are
> more.

Wow, thanks. Fixed (in two places).

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Pool (mbp)]

>> Assuming you've read the comments above, you know we already have cycles involving branches so I'd rather not introduce a new one when I can avoid it.

I've just re-read the preceding comments about cycles.

My first impression here, apparently the same as John, was that this seemed like premature optimization.

It sounds like you have a rule in your head along the lines of "objects that hold references to long-lived parents should always do so through a weakref", or something like that. As spiv said, "this (using weakrefs to avoid cycles) is a new idiom for us and I think it's reasonable to expect we'll make a few mistakes with it."

Could you perhaps post to the list explaining the general rule you think we ought to follow about avoiding cycles? That may save discussing whether it's necessary case-by-case and it will also help other people use it. Also we can discuss whether we prefer that or a .close or .finalize method.

It seems reasonable that the branch config stay in memory as long as the branch is in memory. Is the problem here (per bug 758652) that we suspect Python is just too bad at collecting cyclic structures and we should just always avoid them?

[Martin Pool (mbp)]

OK, I kicked off a thread "weakrefs and avoiding gc cycles"; please reply there.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-05-31 17:31:41 +0000
+++ bzrlib/config.py	2011-05-31 17:31:41 +0000
@@ -2167,6 +2167,15 @@
         """
         raise NotImplementedError(self._load_from_string)
 
+    def unload(self):
+        """Unloads the Store.
+
+        This should make is_loaded() return False. This is used when the caller
+        knows that the persistent storage has changed or may have change since
+        the last load.
+        """
+        raise NotImplementedError(self.unload)
+
     def save(self):
         """Saves the Store to persistent storage."""
         raise NotImplementedError(self.save)
@@ -2220,6 +2229,9 @@
     def is_loaded(self):
         return self._config_obj != None
 
+    def unload(self):
+        self._config_obj = None
+
     def load(self):
         """Load the store from the associated file."""
         if self.is_loaded():
@@ -2370,14 +2382,25 @@
     def __init__(self, branch):
         super(BranchStore, self).__init__(branch.control_transport,
                                           'branch.conf')
-        # FIXME: This creates a cycle -- vila 2011-05-27
-        self.branch = branch
+        # We don't want to create a cycle here when the BranchStore becomes
+        # part of an object (roughly a Stack, directly or indirectly) that is
+        # an attribute of the branch object itself. Since the BranchStore
+        # cannot exist without a branch, it's safe to make it a weakref.
+        self.branch_ref = weakref.ref(branch)
+
+    def _get_branch(self):
+        b = self.branch_ref()
+        if b is None:
+            # Programmer error, a branch store can't exist if the branch it
+            # refers to is dead.
+            raise AssertionError('Dead branch ref in %r' % (self,))
+        return b
 
     def lock_write(self, token=None):
-        return self.branch.lock_write(token)
+        return self._get_branch().lock_write(token)
 
     def unlock(self):
-        return self.branch.unlock()
+        return self._get_branch().unlock()
 
     @needs_write_lock
     def save(self):
@@ -2578,8 +2601,8 @@
     """
 
     def set(self, name, value):
-        # Force a reload (assuming we use a LockableIniFileStore)
-        self.store._config_obj = None
+        # Force a reload
+        self.store.unload()
         super(_CompatibleStack, self).set(name, value)
         # Force a write to persistent storage
         self.store.save()
@@ -2602,7 +2625,6 @@
         super(LocationStack, self).__init__(
             [matcher.get_sections, gstore.get_sections], lstore)
 
-# FIXME: See BranchStore, same remarks -- vila 20110512
 class BranchStack(_CompatibleStack):
 
     def __init__(self, branch):
@@ -2613,6 +2635,7 @@
         super(BranchStack, self).__init__(
             [matcher.get_sections, bstore.get_sections, gstore.get_sections],
             bstore)
+        self.branch = branch
 
 
 class cmd_config(commands.Command):
@@ -2794,7 +2817,7 @@
 # object.
 test_store_builder_registry = registry.Registry()
 
-# Thre registered object should be a callable receiving a test instance
+# The registered object should be a callable receiving a test instance
 # parameter (inheriting from tests.TestCaseWithTransport) and returning a Stack
 # object.
 test_stack_builder_registry = registry.Registry()

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-05-31 17:31:41 +0000
+++ bzrlib/tests/test_config.py	2011-05-31 17:31:41 +0000
@@ -41,6 +41,7 @@
     trace,
     transport,
     )
+from bzrlib.transport import remote
 from bzrlib.tests import (
     features,
     TestSkipped,
@@ -72,33 +73,99 @@
 config.test_store_builder_registry.register(
     'location', lambda test: config.LocationStore())
 
+
+def build_backing_branch(test, relpath,
+                         transport_class=None, server_class=None):
+    """Test helper to create a backing branch only once.
+
+    Some tests needs multiple stores/stacks to check concurrent update
+    behaviours. As such, they need to build different branch *objects* even if
+    they share the branch on disk.
+
+    :param relpath: The relative path to the branch. (Note that the helper
+        should always specify the same relpath).
+
+    :param transport_class: The Transport class the test needs to use.
+
+    :param server_class: The server associated with the ``transport_class``
+        above.
+
+    Either both or neither of ``transport_class`` and ``server_class`` should
+    be specified.
+    """
+    if transport_class is not None and server_class is not None:
+        test.transport_class = transport_class
+        test.transport_server = server_class
+    elif not (transport_class is None and server_class is None):
+        raise AssertionError('Specify both ``transport_class`` and '
+                             '``server_class`` or neither of them')
+    if getattr(test, 'backing_branch', None) is None:
+        # First call, let's build the branch on disk
+        test.backing_branch = test.make_branch(relpath)
+
+
+def keep_branch_alive(test, b):
+    """Keep a branch alive for the duration of a test.
+
+    :param tests: the test that should hold the branch alive.
+
+    :param b: the branch that should be kept alive.
+
+    Several tests need to keep a reference to a branch object as they are
+    testing a Store which uses a weak reference. This is achieved by embedding
+    a reference to the branch object in a lambda passed to a cleanup. When the
+    test finish the cleanup method is deleted and so does the reference to the
+    branch.
+    """
+    test.addCleanup(lambda : b)
+
+
 def build_branch_store(test):
-    if getattr(test, 'branch', None) is None:
-        test.branch = test.make_branch('branch')
-    # Since we can be called to create different stores, we need to build them
-    # from different branch *objects*, even if they point to the same branch on
-    # disk, otherwise tests about conccurent updates won't be able to trigger
-    # LockContention
-    return config.BranchStore(branch.Branch.open('branch'))
+    build_backing_branch(test, 'branch')
+    b = branch.Branch.open('branch')
+    keep_branch_alive(test, b)
+    return config.BranchStore(b)
 config.test_store_builder_registry.register('branch', build_branch_store)
 
 
+def build_remote_branch_store(test):
+    # There is only one permutation (but we won't be able to handle more with
+    # this design anyway)
+    (transport_class, server_class) = remote.get_test_permutations()[0]
+    build_backing_branch(test, 'branch', transport_class, server_class)
+    b = branch.Branch.open(test.get_url('branch'))
+    keep_branch_alive(test, b)
+    return config.BranchStore(b)
+config.test_store_builder_registry.register('remote_branch',
+                                            build_remote_branch_store)
+
+
 config.test_stack_builder_registry.register(
     'bazaar', lambda test: config.GlobalStack())
 config.test_stack_builder_registry.register(
     'location', lambda test: config.LocationStack('.'))
 
+
 def build_branch_stack(test):
-    if getattr(test, 'branch', None) is None:
-        test.branch = test.make_branch('branch')
-    # Since we can be called to create different stacks, we need to build them
-    # from different branch *objects*, even if they point to the same branch on
-    # disk, otherwise tests about conccurent updates won't be able to trigger
-    # LockContention
-    return config.BranchStack(branch.Branch.open('branch'))
+    build_backing_branch(test, 'branch')
+    b = branch.Branch.open('branch')
+    keep_branch_alive(test, b)
+    return config.BranchStack(b)
 config.test_stack_builder_registry.register('branch', build_branch_stack)
 
 
+def build_remote_branch_stack(test):
+    # There is only one permutation (but we won't be able to handle more with
+    # this design anyway)
+    (transport_class, server_class) = remote.get_test_permutations()[0]
+    build_backing_branch(test, 'branch', transport_class, server_class)
+    b = branch.Branch.open(test.get_url('branch'))
+    keep_branch_alive(test, b)
+    return config.BranchStack(b)
+config.test_stack_builder_registry.register('remote_branch',
+                                            build_remote_branch_stack)
+
+
 sample_long_alias="log -r-15..-1 --line"
 sample_config_text = u"""
 [DEFAULT]
@@ -642,11 +709,11 @@
     def test_default_is_True(self):
         self.config = self.get_config(True)
         self.assertExpandIs(True)
-        
+
     def test_default_is_False(self):
         self.config = self.get_config(False)
         self.assertExpandIs(False)
-        
+
 
 class TestIniConfigOptionExpansion(tests.TestCase):
     """Test option expansion from the IniConfig level.
@@ -1940,7 +2007,6 @@
 
     def setUp(self):
         super(TestReadonlyStore, self).setUp()
-        self.branch = self.make_branch('branch')
 
     def test_building_delays_load(self):
         store = self.get_store(self)
@@ -1988,7 +2054,9 @@
         return self.transport.has(store_basename)
 
     def test_save_empty_creates_no_file(self):
-        if self.store_id == 'branch':
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
             raise tests.TestNotApplicable(
                 'branch.conf is *always* created when a branch is initialized')
         store = self.get_store(self)
@@ -2007,7 +2075,9 @@
         self.assertLength(0, sections)
 
     def test_save_with_content_succeeds(self):
-        if self.store_id == 'branch':
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
             raise tests.TestNotApplicable(
                 'branch.conf is *always* created when a branch is initialized')
         store = self.get_store(self)
@@ -2114,7 +2184,28 @@
         self.assertPathExists('dir/subdir')
 
 
+class TestBranchStore(TestStore):
+
+    def test_dead_branch(self):
+        build_backing_branch(self, 'branch')
+        b = branch.Branch.open('branch')
+        store = config.BranchStore(b)
+        del b
+        # The only reliable way to trigger the error is to explicitly call the
+        # garbage collector.
+        import gc
+        gc.collect()
+        store.get_mutable_section(None).set('foo', 'bar')
+        self.assertRaises(AssertionError, store.save)
+
+
 class TestConcurrentStoreUpdates(TestStore):
+    """Test that Stores properly handle conccurent updates.
+
+    New Store implementation may fail some of these tests but until such
+    implementations exist it's hard to properly filter them from the scenarios
+    applied here. If you encounter such a case, contact the bzr devs.
+    """
 
     scenarios = [(key, {'get_stack': builder}) for key, builder
                  in config.test_stack_builder_registry.iteritems()]