std.file: Fix race condition when mkdirRecurse runs concurrently #1818
Conversation
Excellent. |
| { | ||
| if (CreateDirectoryW(std.utf.toUTF16z(pathname), null)) | ||
| return true; | ||
| enum ERROR_ALREADY_EXISTS = 183; |
There was a problem hiding this comment.
Shouldn't this be in druntime somewhere?
There was a problem hiding this comment.
Argh, yes. But I already did two Druntime bindings pulls today :)
I'll move it.
| @@ -1366,6 +1366,25 @@ void mkdir(in char[] pathname) | |||
| } | |||
| } | |||
|
|
|||
| // Ignores "already exists" errors. | |||
| private bool tryMkdir(in char[] pathname) | |||
|
How about: void mkdir(in char[] pathname, bool recursive = false)I agree with the "mkdir -p" behavior of silently accepting existing leaf directories, since whenever I used it, it was to ensure that any missing directory is created. It is the more common use case for me by far. |
mkdirRecurse would crash if a directory was created between its exists() and mkdir() calls. This could occur if mkdirRecurse was called at the same time from mutiple threads or processes. Instead of relying on the exists() check to indicate whether the next mkdir() should succeed, we explicitly ignore "already exists" errors from the OS. Note that this changes the behavior of mkdirRecurse when the leaf directory already existed: previously it would throw, whereas now it will silently succeed. The new behavior is conformant with some other implementations of "recursive mkdir": the -p flag of the GNU tool, Ruby's mkpath, Java's mkdirs, and Perl's make_path. (On the other hand, the old behavior was equivalent to the behavior of Python's makedirs).
|
@mleise: boolean parameters suck. From the call site |
| mkdirRecurse(path); | ||
| mkdirRecurse(path); // should not throw | ||
|
|
||
| assertThrown!FileException(mkdirRecurse(`1:\foobar`)); |
There was a problem hiding this comment.
Hmm not sure what you're testing here, but it's failing the autotester.
There was a problem hiding this comment.
Derp, forgot to put it in version(Windows)
There was a problem hiding this comment.
Ah, 1:\foobar, that's a 1, not an l, which confused me.
But how come on Posix this didn't fail?
There was a problem hiding this comment.
Well, both : and \ are allowed in UNIX filenames...
The left.length != pathname.length check both takes care of the Windows-only check, and it avoids a pointless stat on the FS root.
|
Auto-merge toggled on |
|
LGTM. |
std.file: Fix race condition when mkdirRecurse runs concurrently
ghost
merged commit 
mkdirRecursewould throw if a directory was created between itsexists()andmkdir()calls. This could occur ifmkdirRecursewas called at the same time from mutiple threads or processes.Instead of relying on the
exists()check to indicate whether the nextmkdir()should succeed, we explicitly ignore "already exists" errors from the OS. Note that this changes the behavior ofmkdirRecursewhen the leaf directory already existed: previously it would throw, whereas now it will silently succeed. The new behavior is conformant with some other implementations of "recursive mkdir": the-pflag of the GNU tool, Ruby'smkpath, Java'smkdirs, and Perl'smake_path. (On the other hand, the old behavior was equivalent to the behavior of Python'smakedirs).