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

login
register
mail settings
Submitter Durham Goode
Date May 18, 2017, 6:23 p.m.
Message ID <19be454ee16925d01da8.1495131835@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/20676/
State Deferred
Headers show

Comments

Durham Goode - May 18, 2017, 6:23 p.m.
# HG changeset patch
# User Durham Goode <durham@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().

The fix is to release the lock once we've boostrapped the clone and switch to a
normal repo.lock version.
via Mercurial-devel - May 24, 2017, 8:55 p.m.
On Thu, May 18, 2017 at 11:23 AM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

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()
+                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: