Fix race condition in fs::create_dir_all

[droundy]

The code would crash if the directory was created after create_dir_all
checked whether the directory already existed. This was contrary to
the documentation which claimed to create the directory if it doesn't
exist, implying (but not stating) that there would not be a failure
due to the directory existing.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[brson]

@bors r+ Thanks.

[bors]

📌 Commit 9710b31 has been approved by brson

[bors]

⌛ Testing commit 9710b31 with merge 268dbba...

[alexcrichton]

@bors: r-

Ah sorry I was hoping to discuss this a little more before sending off to bors. These sorts of semantics are pretty subtle and it's often hard to define what a race here is. For example this same error will happen if two threads call create_dir concurrently, but I don't think we should be adding a check there either.

Concurrent operations can't always be detected and when they do happen on the filesystem it often indicates that something else is going awry and needs to be kicked up further. As an example, the boost implementation of this function has the same semantics as this where the error isn't inspected on the way out.

Overall I think that we don't have a concrete enough handle on what a race is for a function like this that I don't think we should add an extra check after-the-fact.

[bors]

💔 Test failed - auto-linux-32-opt

[droundy]

I feel that the after-the-fact check is the only correct way to do this. I
agree that this affects the case where two threads are calling
create_for_all, as well as the less likely case where another program (or
instance of the same program) creates the directory.

In any case, like with mkdir -p, I value this kind of function because it
behaves in an idempotent manner, and dislike this race because it violates
that idempotency property, in that it means that it can fail even when the
directory exists.

For some background, the reason I discovered this problem was a set of test
functions that I wrote which each started by creating the same test
directory so they would be easy to clean up. I was surprised to see
occasional failures due to the create_dir_all failing due to the directory
already existing. So it is a real problem albeit one that I could and will
work around.

David

On Tue, Dec 1, 2015, 6:33 PM Alex Crichton notifications@github.com wrote:

@bors https://github.com/bors: r-

Ah sorry I was hoping to discuss this a little more before sending off to
bors. These sorts of semantics are pretty subtle and it's often hard to
define what a race here is. For example this same error will happen if two
threads call create_dir concurrently, but I don't think we should be
adding a check there either.

Concurrent operations can't always be detected and when they do happen on
the filesystem it often indicates that something else is going awry and
needs to be kicked up further. As an example, the boost implementation of
this function
https://github.com/boostorg/filesystem/blob/a682eaa476cf0b4e992884d32dd2ddcfb0b6b1aa/src/operations.cpp#L938
has the same semantics as this where the error isn't inspected on the way
out.

Overall I think that we don't have a concrete enough handle on what a race
is for a function like this that I don't think we should add an extra check
after-the-fact.

—
Reply to this email directly or view it on GitHub
#30152 (comment).

[droundy]

Oh, to respond to the different behavior with create_dir, I agree that its semantics are different, as the documentation clearly states. If you have two calls to create_dir, it is expected that I've of them must always fail.

In contrast, if create_dir_all fails (I assert), it means that the directory does not exist.

[alexcrichton]

I don't think it's possible for create_dir_all to provide the guarantee that the directory exists when it returns, however, as some other thread could always delete it right after it was created. Along those lines I think that there's not much use in handling one race here but not others such as directories being deleted in the hierarchy while others are being created.

Overall most std::fs functions try to have one system call and be somewhat transactional, but that can't always be guaranteed, so if there are two racing operations there must be external synchronization to work robustly.

[droundy]

I would assert that there is a difference between a race between competing
functions (deleting and creating directories) and there being a race
between two functions attempting to do the same thing, which are designed
in a way to be (almost) idempotent. It is that almost that makes the
current behavior annoying.

On Wed, Dec 2, 2015 at 12:30 PM Alex Crichton notifications@github.com
wrote:

I don't think it's possible for create_dir_all to provide the guarantee
that the directory exists when it returns, however, as some other thread
could always delete it right after it was created. Along those lines I
think that there's not much use in handling one race here but not others
such as directories being deleted in the hierarchy while others are being
created.

Overall most std::fs functions try to have one system call and be
somewhat transactional, but that can't always be guaranteed, so if there
are two racing operations there must be external synchronization to work
robustly.

—
Reply to this email directly or view it on GitHub
#30152 (comment).

[alexcrichton]

Even if we do start classifying races into different categories I'd prefer to not try to handle some and not others, it seems weird that we pick an arbitrary set of racy conditions for each call to handle but other (just as legitimate races) are left up to callers.

[arthurprs]

I had some tests failing because of this. I agree with @alexcrichton reasoning though

[pnkfelix]

I wish we would at least provide better feedback in the Err variant. E.g. tell the caller how many directories got built by the calls that succeeded.

[brson]

Closing pr @alexcrichton

[dpc]

Since I can't reopen, I have created a new issue for this problem #33707 . Please read my arguments and reconsider.