[PATCH 1 of 6] clone: improve locking pattern for hg clone

Martin von Zweigbergk martinvonz at google.com
Wed May 24 16:55:02 EDT 2017


On Thu, May 18, 2017 at 11:23 AM, Durham Goode <durham at fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1495129620 25200
> #      Thu May 18 10:47:00 2017 -0700
> # Node ID 19be454ee16925d01da8f9d329cb48556083da1b
> # Parent  8a87bfc5bebbbe0ac996ac8e047a029eb931af45
> clone: improve locking pattern for hg clone
>
> Previously, in some cases hg.clone() would take the lock via unconventional
> means (since the repo didn't exist yet). If something later on tried to take the
> lock via normal means (repo.lock()), it would enter a permanent deadlock since
> the lock was held by the process, but the repo object didn't realize it since
> the lock was initially taken without using repo.lock().

It sounds like this opens up for a race condition: Someone else could
lock the repo after creation but before exchange, right? That risk
seems tiny (since the repo was just created, no one really knows about
it yet). And even if it happens, I have a feeling that the repo would
just end up empty but valid. So, seems fine to me.

>
> The fix is to release the lock once we've boostrapped the clone and switch to a
> normal repo.lock version.
>
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -586,6 +586,8 @@ def clone(ui, peeropts, source, dest=Non
>              destpeer = peer(srcrepo, peeropts, dest)
>              srcrepo.hook('outgoing', source='clone',
>                            node=node.hex(node.nullid))
> +            release(destlock)
> +            destlock = None
>          else:
>              try:
>                  destpeer = peer(srcrepo or ui, peeropts, dest, create=True)
> @@ -628,52 +630,58 @@ def clone(ui, peeropts, source, dest=Non
>
>          destrepo = destpeer.local()
>          if destrepo:
> -            template = uimod.samplehgrcs['cloned']
> -            fp = destrepo.vfs("hgrc", "w", text=True)
> -            u = util.url(abspath)
> -            u.passwd = None
> -            defaulturl = str(u)
> -            fp.write(template % defaulturl)
> -            fp.close()
> +            wlock = lock = None
> +            try:
> +                wlock = destrepo.wlock()
> +                lock = destrepo.lock()

Others know better, but I thought we preferred to use the context
manager these days. Now that we're on Python 2.7, we can even acquire
both locks with on "with" statement:

  with destrepo.lock() as wlock, destrepo.lock() as lock:

> +                template = uimod.samplehgrcs['cloned']
> +                fp = destrepo.vfs("hgrc", "w", text=True)
> +                u = util.url(abspath)
> +                u.passwd = None
> +                defaulturl = str(u)
> +                fp.write(template % defaulturl)
> +                fp.close()
>
> -            destrepo.ui.setconfig('paths', 'default', defaulturl, 'clone')
> +                destrepo.ui.setconfig('paths', 'default', defaulturl, 'clone')
>
> -            if update:
> -                if update is not True:
> -                    checkout = srcpeer.lookup(update)
> -                uprev = None
> -                status = None
> -                if checkout is not None:
> -                    try:
> -                        uprev = destrepo.lookup(checkout)
> -                    except error.RepoLookupError:
> -                        if update is not True:
> +                if update:
> +                    if update is not True:
> +                        checkout = srcpeer.lookup(update)
> +                    uprev = None
> +                    status = None
> +                    if checkout is not None:
> +                        try:
> +                            uprev = destrepo.lookup(checkout)
> +                        except error.RepoLookupError:
> +                            if update is not True:
> +                                try:
> +                                    uprev = destrepo.lookup(update)
> +                                except error.RepoLookupError:
> +                                    pass
> +                    if uprev is None:
> +                        try:
> +                            uprev = destrepo._bookmarks['@']
> +                            update = '@'
> +                            bn = destrepo[uprev].branch()
> +                            if bn == 'default':
> +                                status = _("updating to bookmark @\n")
> +                            else:
> +                                status = (_("updating to bookmark @ on "
> +                                            "branch %s\n") % bn)
> +                        except KeyError:
>                              try:
> -                                uprev = destrepo.lookup(update)
> +                                uprev = destrepo.branchtip('default')
>                              except error.RepoLookupError:
> -                                pass
> -                if uprev is None:
> -                    try:
> -                        uprev = destrepo._bookmarks['@']
> -                        update = '@'
> +                                uprev = destrepo.lookup('tip')
> +                    if not status:
>                          bn = destrepo[uprev].branch()
> -                        if bn == 'default':
> -                            status = _("updating to bookmark @\n")
> -                        else:
> -                            status = (_("updating to bookmark @ on branch %s\n")
> -                                      % bn)
> -                    except KeyError:
> -                        try:
> -                            uprev = destrepo.branchtip('default')
> -                        except error.RepoLookupError:
> -                            uprev = destrepo.lookup('tip')
> -                if not status:
> -                    bn = destrepo[uprev].branch()
> -                    status = _("updating to branch %s\n") % bn
> -                destrepo.ui.status(status)
> -                _update(destrepo, uprev)
> -                if update in destrepo._bookmarks:
> -                    bookmarks.activate(destrepo, update)
> +                        status = _("updating to branch %s\n") % bn
> +                    destrepo.ui.status(status)
> +                    _update(destrepo, uprev)
> +                    if update in destrepo._bookmarks:
> +                        bookmarks.activate(destrepo, update)
> +            finally:
> +                release(lock, wlock)
>      finally:
>          release(srclock, destlock)
>          if cleandir is not None:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


More information about the Mercurial-devel mailing list