Patchwork scmutil: create directories in a race-safe way during update

login
register
mail settings
Submitter Bryan O'Sullivan
Date Feb. 12, 2013, 12:15 a.m.
Message ID <4034b8d551b116b88292.1360628151@australite.local>
Download mbox | patch
Permalink /patch/964/
State Accepted
Commit 4034b8d551b116b882920466e1d209e475a62721
Headers show

Comments

Bryan O'Sullivan - Feb. 12, 2013, 12:15 a.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1360628112 28800
# Node ID 4034b8d551b116b882920466e1d209e475a62721
# Parent  f12804d3ff801b989cb2aab1aad93047a8db46f1
scmutil: create directories in a race-safe way during update

With the new parallel update code, it is possible for multiple
workers to try to create a hierarchy of directories at the same
time. This is hard to trigger in general, but most likely during
initial checkout.

To deal with these races, we introduce a new ensuredirs function
whose contract is to ensure that a directory hierarchy exists - it
will ignore a failure that implies that the desired directory already
exists.
Thomas Arendsen Hein - Feb. 12, 2013, 12:23 p.m.
* Bryan O'Sullivan <bos@serpentine.com> [20130212 01:16]:
> # HG changeset patch
> # User Bryan O'Sullivan <bryano@fb.com>
> # Date 1360628112 28800
> # Node ID 4034b8d551b116b882920466e1d209e475a62721
> # Parent  f12804d3ff801b989cb2aab1aad93047a8db46f1
> scmutil: create directories in a race-safe way during update

> +def ensuredirs(name, mode=None):
> +    """race-safe recursive directory creation"""
> +    try:
> +        makedirs(name, mode)
> +    except OSError, err:
> +        if err.errno == errno.EEXIST and os.path.isdir(name):
> +            # someone else seems to have won a directory creation race
> +            return
> +        raise
> +

This is not completely race-safe as makedirs uses recursion, which
can abort too early when one worker is much faster than the other:

Concurrent runs of ensuredirs('a/b/c') and ensuredirs('a/b/d'),
initially 'a' does not exist:

Worker 1:                        | Worker 2:
---------------------------------+----------------------------
mkdir1('a/b/c')                  | mkdir1('a/b/d')
-> ENOENT                        | -> ENOENT
makedirs('a/b')                  |
        mkdir1('a/b')            |
        -> ENOENT                |
                                 | makedirs('a/b')
                                 |         mkdir1('a/b')
                                 |         -> ENOENT
                                 |         makedirs('a')
                                 |                 mkdir1('a')
                                 |                 -> OK
                                 |         mkdir2('a/b')
                                 |         -> OK
                                 | mkdir2('d')
        makedirs('a')            |
        mkdir2('a/b')            |
        -> EEXIST                |
mkdir2('a/b/c') is not executed! |
OSError with EEXIST is raised.   |

mkdir1 is the call to os.mkdir in the try part of makedirs(), mkdir2
is the call in the except part.

Regards,
Thomas
Bryan O'Sullivan - Feb. 12, 2013, 5:03 p.m.
On Tue, Feb 12, 2013 at 4:23 AM, Thomas Arendsen Hein
<thomas@intevation.de>wrote:

> This is not completely race-safe as makedirs uses recursion, which
> can abort too early when one worker is much faster than the other:
>

That's a good point. I'll take a second stab at it later this morning.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -307,7 +307,7 @@  class vfs(abstractvfs):
             if basename:
                 if atomictemp:
                     if not os.path.isdir(dirname):
-                        util.makedirs(dirname, self.createmode)
+                        util.ensuredirs(dirname, self.createmode)
                     return util.atomictempfile(f, mode, self.createmode)
                 try:
                     if 'w' in mode:
@@ -326,7 +326,7 @@  class vfs(abstractvfs):
                         raise
                     nlink = 0
                     if not os.path.isdir(dirname):
-                        util.makedirs(dirname, self.createmode)
+                        util.ensuredirs(dirname, self.createmode)
                 if nlink > 0:
                     if self._trustnlink is None:
                         self._trustnlink = nlink > 1 or util.checknlink(f)
@@ -347,7 +347,7 @@  class vfs(abstractvfs):
 
         dirname = os.path.dirname(linkname)
         if not os.path.exists(dirname):
-            util.makedirs(dirname, self.createmode)
+            util.ensuredirs(dirname, self.createmode)
 
         if self._cansymlink:
             try:
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -880,6 +880,16 @@  def makedirs(name, mode=None):
     if mode is not None:
         os.chmod(name, mode)
 
+def ensuredirs(name, mode=None):
+    """race-safe recursive directory creation"""
+    try:
+        makedirs(name, mode)
+    except OSError, err:
+        if err.errno == errno.EEXIST and os.path.isdir(name):
+            # someone else seems to have won a directory creation race
+            return
+        raise
+
 def readfile(path):
     fp = open(path, 'rb')
     try: