| msg58919 - (view) |
Author: Isaac Morland (ijmorlan) |
Date: 2007-12-20 19:49 |
| 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. |
|
|
| msg58921 - (view) |
Author: Raghuram Devarakonda (draghuram)  |
Date: 2007-12-20 19:58 |
| I don't think os.makedirs() can do anything here. It should be caller's responsibility to check for this kind of issues. |
|
|
| msg58922 - (view) |
Author: Isaac Morland (ijmorlan) |
Date: 2007-12-20 19:59 |
| The only thing I found in the bug database concerning os.makedirs was Issue 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 Issue 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. |
|
|
| msg58924 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2007-12-20 20:03 |
| 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() 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. |
|
|
| msg58925 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-20 20:06 |
| 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. |
|
|
| msg58926 - (view) |
Author: Isaac Morland (ijmorlan) |
Date: 2007-12-20 20:13 |
| 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. |
|
|
| msg58950 - (view) |
Author: Isaac Morland (ijmorlan) |
Date: 2007-12-21 18:19 |
| 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. |
|
|
| msg58951 - (view) |
Author: Isaac Morland (ijmorlan) |
Date: 2007-12-21 18:25 |
| 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 |
|
|
| msg75766 - (view) |
Author: Zooko O'Whielacronx (zooko) |
Date: 2008-11-11 22:59 |
| 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... |
|
|
| msg110751 - (view) |
Author: Isaac Morland (ijmorlan) |
Date: 2010-07-19 13:40 |
| This is again being discussed in Issue 9299. |
|
|
| msg110772 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2010-07-19 16:11 |
| The precipitating issue for this and #9299 are different: parent race leading to error versus tail existence leading to error. However, both patches address both issues. See #9299 for my comparison of this patch and that. I am consolidating nosy lists there. Perhaps most/all further discussion should be directed there. |
|
|
| msg110992 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2010-07-21 02:37 |
| 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. |
|
|