[PATCH 2 of 4 STABLE V3] lock: avoid unintentional lock acquisition at failure of readlock

[Yuya Nishihara]

On Mon, 01 May 2017 20:02:50 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> # Date 1493636353 -32400
> #      Mon May 01 19:59:13 2017 +0900
> # Branch stable
> # Node ID cc349553a2126c8e28bc24402bab3bad7981c8c0
> # Parent  e1938d6051da1eff0c0d78fc7aae0d426e99aad2
> lock: avoid unintentional lock acquisition at failure of readlock
> 
> Acquiring lock by vfs.makelock() and getting lock holder (aka
> "locker") information by vfs.readlock() aren't atomic action.
> Therefore, failure of the former doesn't ensure success of the latter.
> 
> Before this patch, lock is unintentionally acquired, if ENOEXIST
> causes failure of vfs.readlock() while 5 times retrying, because
> lock._trylock() returns to caller silently after retrying, and
> lock.lock() assumes that lock._trylock() returns successfully only if
> lock is acquired.
> 
> In this case, lock symlink (or file) isn't created, even though lock
> is treated as acquired in memory.
> 
> To avoid this issue, this patch makes lock._trylock() raise
> LockHeld(EAGAIN) at the end of it, if lock isn't acquired while
> retrying.
> 
> An empty "locker" meaning "busy for frequent lock/unlock by many
> processes" might appear in an abortion message, if lock acquisition
> fails. Therefore, this patch also does:
> 
>   - use '%r' to increase visibility of "locker", even if it is empty
>   - show hint message to explain what empty "locker" means
> 
> diff --git a/mercurial/lock.py b/mercurial/lock.py
> --- a/mercurial/lock.py
> +++ b/mercurial/lock.py
> @@ -151,6 +151,12 @@ class lock(object):
>                      raise error.LockUnavailable(why.errno, why.strerror,
>                                                  why.filename, self.desc)
>  
> +        if not self.held:
> +            # use empty locker to mean "busy for frequent lock/unlock
> +            # by many processes"
> +            raise error.LockHeld(errno.EAGAIN,
> +                                 self.vfs.join(self.f), self.desc, "")

For reference, this bug would probably exist since 3b6e5914edd8 "lock: loop
a finite number of times in trylock (issue4787)." Before, trylock() would
never return until lock was taken.