[0.18][#109169][merge] add lock debugging, remove unnecessary lock peeks

[Martin Pool]

On 7/4/07, Robert Collins <robertc at robertcollins.net> wrote:
> On Wed, 2007-07-04 at 19:24 +1000, Martin Pool wrote:
> >
> > > Why not create brokenrename transport and server within the test
> > module?
> > > Its testing code, and you even have a comment that you don't want it
> > to
> > > be required to pass the transport conformance tests.
> >
> > Do you mean moving it to tests.transport.brokenrename or something?
>
> I mean in the lock tests python file - you only need the Server
> declaration and the TransportDecorator subclass.

It could be useful elsewhere if other code needs to handle this case,
and it's sufficiently conceptually separate that I don't think it
needs to be in the same place.  We've had duplication in the test
suite in the past for several reasons, but one of them was that
general code was put just in the place that needed it at the time, and
I don't want to repeat that.

> > Actually I think it would/should pass the tests, but it is so trivial
> > as to probably not be worth running through every test.  I  put it
> > there because that's where vfat and similar things are.
>
> Well, untested code is broken code :).

I tested it by observing that it did cause the expected test failure
when I introduced it.  It could have a specific test but for now it
didn't seem needed.

> > Mostly because I didn't want to make things slower on sftp, and we
> > should rarely need to do the delete_tree - only on servers with that
> > rename behaviour and only when there was lock contention.  On the
> > other hand just always doing the recursive deletion is simpler.
>
> Right. And in fact we could have the sftp recursive delete try a regular
> delete first and see if that worked.

We could pass it a list of "here are the files I expect will exist, so
delete them first before listing the directory."  But I can't think
off hand of any other place that would want to do that.

> > > I note that you still double-peek - why is that ?
> >
> > Where?
>
> after _attempt_lock it says 'sometimes we know that its held by someone
> else but we peek anyway' or words to that effect. Perhaps a comment
> saying that this is not a performance issue because XXX.

The comment as merged says

            # TODO: In a few cases, we find out that there's contention by
            # reading the held info and observing that it's not ours.  In
            # those cases it's a bit redundant to read it again.  However,
            # the normal case (??) is that the rename fails and so we
            # don't know who holds the lock.  For simplicity we peek
            # always.

we could only rarely avoid peeking twice, so we do it unconditionally.
 If that's unclear I can change the comment, or if we observe it
actually happens often we can change it.

So for now I'd like to leave this, it could be tweaked more but I've
basically done what I wanted which is to make it possible to see
what's going on, and to fix the cruft-leaving bug.

-- 
Martin