[PATCH STABLE v2] hg: obtain lock when creating share from pooled repo (issue5104)
Gregory Szorc
gregory.szorc at gmail.com
Sun Feb 28 02:26:13 UTC 2016
On Sat, Feb 27, 2016 at 6:21 PM, Gregory Szorc <gregory.szorc at gmail.com>
wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1456625878 28800
> # Sat Feb 27 18:17:58 2016 -0800
> # Branch stable
> # Node ID 37ba948d189f0f8b3a0726c4c89038a452d6fd4c
> # Parent cb6a952efbf48d306a7c82d7fe46115f072cbb1d
> 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.
>
> To test this, a simple generic "lockdelay" extension has been added.
> The extension inserts an optional, configurable delay before or after
> lock acquisition. In the test, we delay 2 seconds after lock acquisition
> in the first process and 1 second before lock acquisition in the 2nd
> process. This means the first process has 1s to obtain the lock. There
> is a race condition here. If we encounter it in the wild, we could
> change the dummy extension to wait on the lock file to appear instead
> of relying on timing. But that's more complicated. Let's see what
> happens first.
>
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -330,27 +330,40 @@ 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)
>
> - 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 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)
>
> 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/lockdelay.py b/tests/lockdelay.py
> new file mode 100644
> --- /dev/null
> +++ b/tests/lockdelay.py
> @@ -0,0 +1,26 @@
> +# Dummy extension that adds a delay after acquiring a lock.
> +#
> +# This extension can be used to test race conditions between lock
> acquisition.
> +
> +from __future__ import absolute_import
> +
> +import os
> +import time
> +
> +from mercurial import (
> + lock as lockmod,
> +)
> +
> +class delaylock(lockmod.lock):
> + def lock(self):
> + delay = float(os.environ.get('HGPRELOCKDELAY', '0.0'))
> + if delay:
> + time.sleep(delay)
> + res = super(delaylock, self).lock()
> + delay = float(os.environ.get('HGPOSTLOCKDELAY', '0.0'))
> + if delay:
> + time.sleep(delay)
> + return res
> +
> +def extsetup(ui):
> + lockmod.lock = delaylock
> \ No newline at end of file
>
Oops. This upsets test-check-code.t. I'll send a V3.
> 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,54 @@ 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)
> +
> + $ HGPOSTLOCKDELAY=2.0 hg --config share.pool=racepool --config
> extensions.lockdelay=$TESTDIR/lockdelay.py clone source1a share-destrace1 >
> race1.log 2>&1 &
> + $ HGPRELOCKDELAY=1.0 hg --config share.pool=racepool --config
> extensions.lockdelay=$TESTDIR/lockdelay.py clone source1a share-destrace2
> > race2.log 2>&1
> + $ wait
> +
> + $ 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
> +
> + $ cat race1.log
> + (sharing from new pooled repository
> b5f04eac9d8f7a6a9fcb070243cccea7dc5ea0c1)
> + requesting all changes
> + adding changesets
> + adding manifests
> + adding file changes
> + added 3 changesets with 3 changes to 1 files
> + updating working directory
> + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + searching for changes
> + no changes found
> + adding remote bookmark bookA
> +
> + $ cat race2.log
> + (sharing from existing pooled repository
> b5f04eac9d8f7a6a9fcb070243cccea7dc5ea0c1)
> + updating working directory
> + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + waiting for lock on repository share-destrace2 held by * (glob)
> + got lock after \d+ seconds (re)
> + searching for changes
> + no changes found
> + adding remote bookmark bookA
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160227/adfb2a1e/attachment-0002.html>
More information about the Mercurial-devel
mailing list