Code review comment for lp:~vila/bzr/config-lock-remote

>>>>> 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 isn't really
    > worth making this code harder to follow on the off chance that we
    > clean other stuff up.)

Harder ? Once you understand what a weakref is, I fail to understand
what is hard to follow in *three* lines of code that uses it, especially
when there is a comment explaining the rationale right above the place
the weakref is created.

I'm sure you've dealt with far harder reference counting issues in
pyrex.

Premature ? I decided to address the cycle issue *now* rather than
later, when I'll start deploying, to outline these 3 lines of code so
they are obvious to understand instead of hidden in a bigger proposal
where they may have been unnoticed...

    > 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().

For which you'll get a traceback which will at least make it clear where
the config can't be used which itself should give some good hints about
why the branch object is dead when its attribute is still alive (since
that's the only planned use case, any other being bogus by definition,
even the test artificially makes the branch lives longer).

    > It is obvious to you *now*, will it be obvious in 2 years

As long as the config is a branch attribute and the branch a config
attribute this would stay obvious to anybody reading:

        # 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.

which I will clarify once I deploy config as a tree/branch/repo
attribute.

    > 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.

As long as the branch is alive, the config is alive. As soon as the
branch dies, so does the config. That's exactly what weakref is for so
we don't have to worry about ref counting and in this case about cycles.

I'm more worried about having to find how/where/when/ cycles are
created.

If you know about existing cycles involving branches, please file bugs !
The sooner we address them the better.