[PATCH] localrepo: Add locking to _branchtags around _writebrancache

[Mads]

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