[PATCH] localrepo: Add locking to _branchtags around _writebrancache

Joshua Redstone joshua.redstone at fb.com
Fri May 25 09:31:26 CDT 2012


Great point about the pointlessness of write locking when the data read is
outside the lock.
My goal was to prevent something like 'hg heads' from racing to update the
branchheads cache versus some other command like
'hg commit', resulting in a very out-of-date cache and a cache that is
potentially corrupt in the sense that we won't detect it is invalid.

I think you're right that to do this right, the read code path really
needs to be inside the lock as well.

I was using wlock (despite the comment) because I thought wlock is what
protects the branchheads cache.

I'll rework this diff.
Josh


On 5/25/12 7:46 AM, "Mads" <mads at kiilerich.com> wrote:

>On 24/05/12 16:50, Joshua Redstone wrote:
>> # HG changeset patch
>> # User Joshua Redstone<joshua.redstone at fb.com>
>> # Date 1336812429 25200
>> # Node ID b3bb1d7360c481b847431fc0d264111b1d93f993
>> # Parent  2ac08d8b21aa7b6e0a062afed5a3f357ccef67f9
>> localrepo:  Add locking to _branchtags around _writebrancache
>
>minor typo
>
>> Read code paths such as via localrepo.branchmap() may end up calling
>> _writebranchcache without having acquire a repo lock, potentially
>>leading to
>> races with other repo mutations.  This fix acquires the repo lock if
>>not yet
>> held before calling _writebranchcache.
>>
>> diff -r 2ac08d8b21aa -r b3bb1d7360c4 mercurial/localrepo.py
>> --- a/mercurial/localrepo.py	Tue May 22 14:37:20 2012 -0500
>> +++ b/mercurial/localrepo.py	Sat May 12 01:47:09 2012 -0700
>> @@ -481,8 +481,18 @@
>>           if lrev != tiprev:
>>               ctxgen = (self[r] for r in xrange(lrev + 1, tiprev + 1))
>>               self._updatebranchcache(partial, ctxgen)
>> -            self._writebranchcache(partial, self.changelog.tip(),
>>tiprev)
>> -
>> +            # Read code paths (e.g., from branchmap()) do not have the
>>lock
>> +            # yet.  Lock acquisition may fail if we do not have
>>write-access
>> +            # to the directory.
>> +            try:
>> +                wlock = self.wlock(wait=False)
>> +                try:
>> +                    self._writebranchcache(partial,
>>self.changelog.tip(),
>> +                                           tiprev)
>> +                finally:
>> +                    wlock.release()
>> +            except error.LockError:
>> +                pass
>>           return partial
>>
>>       def updatebranchcache(self):
>> diff -r 2ac08d8b21aa -r b3bb1d7360c4 mercurial/statichttprepo.py
>
>lock might have been taking before we take wlock here. That is the wrong
>order ... but I guess that is ok and doesn't cause any other problems
>than breaking the rules and being spotted by lock validation ;-)
>
>But I guess the branch cache is a pure function of the repo in store, so
>shouldn't the cache be in store too and be protected by lock instead of
>wlock? The comments indicate that your intent was to use lock instead of
>wlock.
>
>But ... what is the purpose of this lock around _writebranchcache?
>_writebranchcache will use atomictemp anyway and will thus either update
>the cache or leave it untouched.
>
>The bigger problem is that it (apparently) will write something based on
>information it read before taking the lock. Ideally _readbranchcache
>should be inside the locked region as well ... and there is not much
>point in locking as long as it isn't.
>
>> diff -r 2ac08d8b21aa -r b3bb1d7360c4 tests/test-bheads.t
>> --- a/tests/test-bheads.t	Tue May 22 14:37:20 2012 -0500
>> +++ b/tests/test-bheads.t	Sat May 12 01:47:09 2012 -0700
>> @@ -31,6 +31,14 @@
>>
>>   =======
>>
>> +Not being able to update the branchheads cache should not make read
>>ops fail
>> +  $ echo "junk">  $TESTTMP/a/.hg/cache/branchheads
>> +  $ chmod -R u-w $TESTTMP/a/.hg
>> +  $ heads
>> +  1: Adding a branch (a)
>> +  0: Adding root node ()
>> +  $ chmod -R u+w $TESTTMP/a/.hg
>
>That test will fail on windows - it should use hghave unix-permissions.
>
>/Mads



More information about the Mercurial-devel mailing list