Race condition in os.makedirs

[ijmorlan]

BPO	1675
Nosy	@gvanrossum, @terryjreedy
Superseder	bpo-9299: os.makedirs(): Add a keyword argument to suppress "File exists" exception
Files	makedirs.py patch.txt

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2010-07-21.02:13:26.658>
created_at = <Date 2007-12-20.19:49:31.319>
labels = ['easy', 'type-feature', 'library']
title = 'Race condition in os.makedirs'
updated_at = <Date 2010-07-21.02:37:05.326>
user = 'https://bugs.python.org/ijmorlan'

bugs.python.org fields:

activity = <Date 2010-07-21.02:37:05.326>
actor = 'terry.reedy'
assignee = 'none'
closed = True
closed_date = <Date 2010-07-21.02:13:26.658>
closer = 'terry.reedy'
components = ['Library (Lib)']
creation = <Date 2007-12-20.19:49:31.319>
creator = 'ijmorlan'
dependencies = []
files = ['9016', '9022']
hgrepos = []
issue_num = 1675
keywords = ['easy']
message_count = 12.0
messages = ['58919', '58921', '58922', '58924', '58925', '58926', '58950', '58951', '75766', '110751', '110772', '110992']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'terry.reedy', 'zooko', 'ggenellina', 'draghuram', 'ijmorlan']
pr_nums = []
priority = 'low'
resolution = 'duplicate'
stage = 'patch review'
status = 'closed'
superseder = '9299'
type = 'enhancement'
url = 'https://bugs.python.org/issue1675'
versions = ['Python 3.2']

[ijmorlan]

There appears to be a race condition in os.makedirs. Suppose two
processes simultaneously try to create two different directories with a
common non-existent ancestor. For example process 1 tries to create
"a/b" and process 2 tries to create "a/c". They both check that "a"
does not exist, then both invoke makedirs on "a". One of these will
throw OSError (due to the underlying EEXIST system error), and this
exception will be propagated. Note that this happens even though the
two processes are trying to create two different directories and so one
would not expect either to report a problem with the directory already
existing.

[draghuram]

I don't think os.makedirs() can do anything here. It should be caller's
responsibility to check for this kind of issues.

[ijmorlan]

The only thing I found in the bug database concerning os.makedirs was
bpo-766910 (http://bugs.python.org/issue766910). I realized
os.makedirs had a race condition because in my application I want to
create directories but it's perfectly fine if they already exist. This
is exactly what trace.py in bpo-766910 seems to need.

I started writing my own, which was basically just os.makedirs but
calling my own version of os.mkdir which didn't worry about
already-existing directories, but realized that wouldn't work.
Eventually I ended up with the routines I've put in the attached
makedirs.py.

I think os.makedirs can be fixed by making what is now its recursive
call instead call my version of makedirs. I also think both my mkdir
and my makedirs should be present in the standard library as well as the
existing versions. Possibly this could be done by adding a flag to the
existing versions, defaulted to obtain the current behaviour.

[gvanrossum]

I think we can fix this as follows: whenever it calls os.mkdir() and an
error is returned, check if that is EISDIR or EEXISTS, and if so, check
that indeed it now exists as a directory, and then ignore the error.

Moreover, I'd like to do this for the ultimate path to be created as
well, so that os.makedirs(<existing directory>) will succeed instead of
failing. This would make the common usage pattern much simpler.

I think it should still fail if the path exists as a file though. (Or
as a symlink to a file.)

Patch welcome!

I think this is a feature request and hence should only be fixed in 2.6.

[gvanrossum]

Can you rephrase this as svn diff output?

Also, mkdir() is a confusing name for the helper -- I'd call it
forgiving_mkdir() or something like that.

[ijmorlan]

Yes, I'm really combining two things here - the race condition, which I
argue is a (minor) bug, and a feature request to be able to "ensure
exists" a directory.

I have not produced a proper Python patch before and I have other things
to do so this will take longer than one might hope, but I would be happy
to create a patch. Note too that the file I uploaded is from my
project; I will attempt to make the patch be more appropriate for the
standard library than an extract from my project.

[ijmorlan]

Attached is an svn diff against the trunk. I was looking at os.py from
Python 2.5 not the trunk, and it appears that an attempt at fixing the
race condition has already been put into os.py, but I don't believe it's
correct.

The attached patch renames the existing mkdir to _mkdir, and creates a
new mkdir with an additional "excl" parameter to select
error-if-already-exists or not. It defaults to the current behaviour.
Similarly, makedirs gets the same extra parameter which is passed down
to mkdir.

By simply using the new versions as before, one obtains the old
behaviour unchanged except that the race condition is corrected. By
using excl=False one gets the new behaviour.

I have updated the documentation also but I don't really know what I'm
doing there so my use of the rst format may not be right.

[ijmorlan]

I should add that the new parameter is called "excl" by analogy with the
O_EXCL option to os.open().

Also, I'm not absolutely certain about the test for which exceptions
should be ignored when excl == False:

e.errno == errno.EEXIST and path.isdir (name)

This will not work if errno is set to something other than EEXIST when
mkdir fails due to the directory already existing. The above works on
my system but I can't be certain that all mkdir implementations report
EEXIST.

It should be safe to drop the errno check altogether, and I'm starting
to think that we should; at present it's really just an optimization to
avoid using .isdir, but only in what should be rather uncommon
circumstances. I think the smell of "premature optimization" may be
hard to ignore.

So the if statement would be:

if excl or not path.isdir (name):
    raise

[zooko]

Here's the version of this that I've been using for almost a decade now:

http://allmydata.org/trac/pyutil/browser/pyutil/pyutil/fileutil.py?rev=127#L241

Actually I used to have a bigger version that could optionally require
certain things of the mode of the directory, but it turned out that I
wasn't going to need it.

def make_dirs(dirname, mode=0777):
    """
    An idempotent version of os.makedirs().  If the dir already exists, do
    nothing and return without raising an exception.  If this call
creates the
    dir, return without raising an exception.  If there is an error that
    prevents creation or if the directory gets deleted after make_dirs()
creates
    it and before make_dirs() checks that it exists, raise an exception.
    """
    tx = None
    try:
        os.makedirs(dirname, mode)
    except OSError, x:
        tx = x

    if not os.path.isdir(dirname):
        if tx:
            raise tx
        raise exceptions.IOError, "unknown error prevented creation of
directory, or deleted the directory immediately after creation: %s" %
dirname # careful not to construct an IOError with a 2-tuple, as that
has a special meaning...

[ijmorlan]

This is again being discussed in bpo-9299.

[terryjreedy]

The precipitating issue for this and bpo-9299 are different: parent race leading to error versus tail existence leading to error. However, both patches address both issues.

See bpo-9299 for my comparison of this patch and that.

I am consolidating nosy lists there. Perhaps most/all further discussion should be directed there.

[terryjreedy]

Isaac, thank you for the report and patch. With more attention, this might have been tweaked and applied a couple of years ago. We are trying to get better at timely responses.