Patchwork [STABLE] hg: obtain lock when creating share from pooled repo (issue5104)

login
register
mail settings
Submitter Gregory Szorc
Date Feb. 21, 2016, 3:10 a.m.
Message ID <157dff6560d4f1d61947.1456024252@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/13282/
State Superseded
Commit d493d64757eb45ada99fcb3693e479a51b7782da
Delegated to: Pierre-Yves David
Headers show

Comments

Gregory Szorc - Feb. 21, 2016, 3:10 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1456023841 28800
#      Sat Feb 20 19:04:01 2016 -0800
# Branch stable
# Node ID 157dff6560d4f1d61947815f8f2b93b07fc0e39f
# Parent  4f8ced23345e61dda5b26b2db61a9461322201f9
hg: obtain lock when creating share from pooled repo (issue5104)

There are race conditions between clients performing a shared clone
to pooled storage:

1) Clients race to create the new shared repo in the pool directory
2) 1 client is seeding the repo in the pool directory and another goes
   to share it before it is fully cloned

We prevent these race conditions by obtaining a lock in the pool
directory that is derived from the name of the repo we will be
accessing.
Pierre-Yves David - Feb. 24, 2016, 11:07 p.m.
On 02/21/2016 04:10 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1456023841 28800
> #      Sat Feb 20 19:04:01 2016 -0800
> # Branch stable
> # Node ID 157dff6560d4f1d61947815f8f2b93b07fc0e39f
> # Parent  4f8ced23345e61dda5b26b2db61a9461322201f9
> hg: obtain lock when creating share from pooled repo (issue5104)
>
> There are race conditions between clients performing a shared clone
> to pooled storage:
>
> 1) Clients race to create the new shared repo in the pool directory
> 2) 1 client is seeding the repo in the pool directory and another goes
>     to share it before it is fully cloned
>
> We prevent these race conditions by obtaining a lock in the pool
> directory that is derived from the name of the repo we will be
> accessing.

Logic seems overall good. Having some code question below.

>
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -330,27 +330,41 @@ def clonewithshare(ui, peeropts, sharepa
>       revs = None
>       if rev:
>           if not srcpeer.capable('lookup'):
>               raise error.Abort(_("src repository does not support "
>                                  "revision lookup and so doesn't "
>                                  "support clone by revision"))
>           revs = [srcpeer.lookup(r) for r in rev]
>
> +    # Obtain a lock before checking for or cloning the pooled repo otherwise
> +    # 2 clients may race creating or populating it.
> +    pooldir = os.path.dirname(sharepath)
> +    # lock class requires the directory to exist.
> +    try:
> +        util.makedir(pooldir, False)
> +    except OSError as e:
> +        if e.errno != errno.EEXIST:
> +            raise
> +
> +    poolvfs = scmutil.vfs(pooldir)
>       basename = os.path.basename(sharepath)
> +    sharelock = lock.lock(poolvfs, '%s.lock' % basename)
>
> -    if os.path.exists(sharepath):
> -        ui.status(_('(sharing from existing pooled repository %s)\n') %
> -                  basename)
> -    else:
> -        ui.status(_('(sharing from new pooled repository %s)\n') % basename)
> -        # Always use pull mode because hardlinks in share mode don't work well.
> -        # Never update because working copies aren't necessary in share mode.
> -        clone(ui, peeropts, source, dest=sharepath, pull=True,
> -              rev=rev, update=False, stream=stream)
> +    with sharelock:

Why creating the sharelock in a temporary variable on not just in the 
with statement itself. (That would somewhat enforce the lifecycle better).

> +        if os.path.exists(sharepath):
> +            ui.status(_('(sharing from existing pooled repository %s)\n') %
> +                      basename)
> +        else:
> +            ui.status(_('(sharing from new pooled repository %s)\n') % basename)
> +            # Always use pull mode because hardlinks in share mode don't work
> +            # well. Never update because working copies aren't necessary in
> +            # share mode.
> +            clone(ui, peeropts, source, dest=sharepath, pull=True,
> +                  rev=rev, update=False, stream=stream)
>
>       sharerepo = repository(ui, path=sharepath)
>       share(ui, sharerepo, dest=dest, update=update, bookmarks=False)
>
>       # We need to perform a pull against the dest repo to fetch bookmarks
>       # and other non-store data that isn't shared by default. In the case of
>       # non-existing shared repo, this means we pull from the remote twice. This
>       # is a bit weird. But at the time it was implemented, there wasn't an easy
> diff --git a/tests/test-clone.t b/tests/test-clone.t
> --- a/tests/test-clone.t
> +++ b/tests/test-clone.t
> @@ -1020,8 +1020,35 @@ Test that auto sharing doesn't cause fai
>     $ hg -R a id -r 0
>     acb14030fe0a
>     $ hg id -R remote -r 0
>     abort: repository remote not found!
>     [255]
>     $ hg --config share.pool=share -q clone -e "python \"$TESTDIR/dummyssh\"" a ssh://user@dummy/remote
>     $ hg -R remote id -r 0
>     acb14030fe0a
> +
> +Cloning into pooled storage doesn't race (issue5104)
> +
> +  $ hg --config share.pool=racepool clone source1a share-destrace1 > race1.log &
> +  $ hg --config share.pool=racepool clone source1a share-destrace2 > race2.log
> +  $ wait

How are you making sure we race in the tests?
Also, what's the expected message in case of lock holding?
Gregory Szorc - Feb. 28, 2016, 2:22 a.m.
On Wed, Feb 24, 2016 at 3:07 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 02/21/2016 04:10 AM, Gregory Szorc wrote:
>
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1456023841 28800
>> #      Sat Feb 20 19:04:01 2016 -0800
>> # Branch stable
>> # Node ID 157dff6560d4f1d61947815f8f2b93b07fc0e39f
>> # Parent  4f8ced23345e61dda5b26b2db61a9461322201f9
>> hg: obtain lock when creating share from pooled repo (issue5104)
>>
>> There are race conditions between clients performing a shared clone
>> to pooled storage:
>>
>> 1) Clients race to create the new shared repo in the pool directory
>> 2) 1 client is seeding the repo in the pool directory and another goes
>>     to share it before it is fully cloned
>>
>> We prevent these race conditions by obtaining a lock in the pool
>> directory that is derived from the name of the repo we will be
>> accessing.
>>
>
> Logic seems overall good. Having some code question below.
>
>
>
>> diff --git a/mercurial/hg.py b/mercurial/hg.py
>> --- a/mercurial/hg.py
>> +++ b/mercurial/hg.py
>> @@ -330,27 +330,41 @@ def clonewithshare(ui, peeropts, sharepa
>>       revs = None
>>       if rev:
>>           if not srcpeer.capable('lookup'):
>>               raise error.Abort(_("src repository does not support "
>>                                  "revision lookup and so doesn't "
>>                                  "support clone by revision"))
>>           revs = [srcpeer.lookup(r) for r in rev]
>>
>> +    # Obtain a lock before checking for or cloning the pooled repo
>> otherwise
>> +    # 2 clients may race creating or populating it.
>> +    pooldir = os.path.dirname(sharepath)
>> +    # lock class requires the directory to exist.
>> +    try:
>> +        util.makedir(pooldir, False)
>> +    except OSError as e:
>> +        if e.errno != errno.EEXIST:
>> +            raise
>> +
>> +    poolvfs = scmutil.vfs(pooldir)
>>       basename = os.path.basename(sharepath)
>> +    sharelock = lock.lock(poolvfs, '%s.lock' % basename)
>>
>> -    if os.path.exists(sharepath):
>> -        ui.status(_('(sharing from existing pooled repository %s)\n') %
>> -                  basename)
>> -    else:
>> -        ui.status(_('(sharing from new pooled repository %s)\n') %
>> basename)
>> -        # Always use pull mode because hardlinks in share mode don't
>> work well.
>> -        # Never update because working copies aren't necessary in share
>> mode.
>> -        clone(ui, peeropts, source, dest=sharepath, pull=True,
>> -              rev=rev, update=False, stream=stream)
>> +    with sharelock:
>>
>
> Why creating the sharelock in a temporary variable on not just in the with
> statement itself. (That would somewhat enforce the lifecycle better).
>
>
> +        if os.path.exists(sharepath):
>> +            ui.status(_('(sharing from existing pooled repository
>> %s)\n') %
>> +                      basename)
>> +        else:
>> +            ui.status(_('(sharing from new pooled repository %s)\n') %
>> basename)
>> +            # Always use pull mode because hardlinks in share mode don't
>> work
>> +            # well. Never update because working copies aren't necessary
>> in
>> +            # share mode.
>> +            clone(ui, peeropts, source, dest=sharepath, pull=True,
>> +                  rev=rev, update=False, stream=stream)
>>
>>       sharerepo = repository(ui, path=sharepath)
>>       share(ui, sharerepo, dest=dest, update=update, bookmarks=False)
>>
>>       # We need to perform a pull against the dest repo to fetch bookmarks
>>       # and other non-store data that isn't shared by default. In the
>> case of
>>       # non-existing shared repo, this means we pull from the remote
>> twice. This
>>       # is a bit weird. But at the time it was implemented, there wasn't
>> an easy
>> diff --git a/tests/test-clone.t b/tests/test-clone.t
>> --- a/tests/test-clone.t
>> +++ b/tests/test-clone.t
>> @@ -1020,8 +1020,35 @@ Test that auto sharing doesn't cause fai
>>     $ hg -R a id -r 0
>>     acb14030fe0a
>>     $ hg id -R remote -r 0
>>     abort: repository remote not found!
>>     [255]
>>     $ hg --config share.pool=share -q clone -e "python
>> \"$TESTDIR/dummyssh\"" a ssh://user@dummy/remote
>>     $ hg -R remote id -r 0
>>     acb14030fe0a
>> +
>> +Cloning into pooled storage doesn't race (issue5104)
>> +
>> +  $ hg --config share.pool=racepool clone source1a share-destrace1 >
>> race1.log &
>> +  $ hg --config share.pool=racepool clone source1a share-destrace2 >
>> race2.log
>> +  $ wait
>>
>
> How are you making sure we race in the tests?
> Also, what's the expected message in case of lock holding?
>

Feedback addressed in v2, which I just patchbombed.

Patch

diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -330,27 +330,41 @@  def clonewithshare(ui, peeropts, sharepa
     revs = None
     if rev:
         if not srcpeer.capable('lookup'):
             raise error.Abort(_("src repository does not support "
                                "revision lookup and so doesn't "
                                "support clone by revision"))
         revs = [srcpeer.lookup(r) for r in rev]
 
+    # Obtain a lock before checking for or cloning the pooled repo otherwise
+    # 2 clients may race creating or populating it.
+    pooldir = os.path.dirname(sharepath)
+    # lock class requires the directory to exist.
+    try:
+        util.makedir(pooldir, False)
+    except OSError as e:
+        if e.errno != errno.EEXIST:
+            raise
+
+    poolvfs = scmutil.vfs(pooldir)
     basename = os.path.basename(sharepath)
+    sharelock = lock.lock(poolvfs, '%s.lock' % basename)
 
-    if os.path.exists(sharepath):
-        ui.status(_('(sharing from existing pooled repository %s)\n') %
-                  basename)
-    else:
-        ui.status(_('(sharing from new pooled repository %s)\n') % basename)
-        # Always use pull mode because hardlinks in share mode don't work well.
-        # Never update because working copies aren't necessary in share mode.
-        clone(ui, peeropts, source, dest=sharepath, pull=True,
-              rev=rev, update=False, stream=stream)
+    with sharelock:
+        if os.path.exists(sharepath):
+            ui.status(_('(sharing from existing pooled repository %s)\n') %
+                      basename)
+        else:
+            ui.status(_('(sharing from new pooled repository %s)\n') % basename)
+            # Always use pull mode because hardlinks in share mode don't work
+            # well. Never update because working copies aren't necessary in
+            # share mode.
+            clone(ui, peeropts, source, dest=sharepath, pull=True,
+                  rev=rev, update=False, stream=stream)
 
     sharerepo = repository(ui, path=sharepath)
     share(ui, sharerepo, dest=dest, update=update, bookmarks=False)
 
     # We need to perform a pull against the dest repo to fetch bookmarks
     # and other non-store data that isn't shared by default. In the case of
     # non-existing shared repo, this means we pull from the remote twice. This
     # is a bit weird. But at the time it was implemented, there wasn't an easy
diff --git a/tests/test-clone.t b/tests/test-clone.t
--- a/tests/test-clone.t
+++ b/tests/test-clone.t
@@ -1020,8 +1020,35 @@  Test that auto sharing doesn't cause fai
   $ hg -R a id -r 0
   acb14030fe0a
   $ hg id -R remote -r 0
   abort: repository remote not found!
   [255]
   $ hg --config share.pool=share -q clone -e "python \"$TESTDIR/dummyssh\"" a ssh://user@dummy/remote
   $ hg -R remote id -r 0
   acb14030fe0a
+
+Cloning into pooled storage doesn't race (issue5104)
+
+  $ hg --config share.pool=racepool clone source1a share-destrace1 > race1.log &
+  $ hg --config share.pool=racepool clone source1a share-destrace2 > race2.log
+  $ wait
+
+  $ grep -h 'sharing from existing pooled repository' race1.log race2.log
+  (sharing from existing pooled repository b5f04eac9d8f7a6a9fcb070243cccea7dc5ea0c1)
+
+  $ hg -R share-destrace1 log -r tip
+  changeset:   2:e5bfe23c0b47
+  bookmark:    bookA
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     1a
+  
+
+  $ hg -R share-destrace2 log -r tip
+  changeset:   2:e5bfe23c0b47
+  bookmark:    bookA
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     1a
+