Fix race condition in fs::create_dir_all by dpc · Pull Request #39799 · rust-lang/rust

dpc · 2017-02-14T01:28:16Z

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 · 2017-02-14T01:28:26Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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.

dpc · 2017-02-14T01:29:39Z

This is resubmission of #30152 , that I've discussed in: #33707 .

I'd like to add a test for it, but how exactly should it work?

alexcrichton · 2017-02-14T15:17:57Z

Thanks for the PR! Could this actually lift the implementation in the compiler which I believe tries to minimize syscalls in the "normal" case.

Also yeah you can just add a test with a few threads hammering at creating directories and just make sure they don't fail.

seppo0010 · 2017-02-14T15:23:24Z

There's still room for a race condition if the directory is created after mkdir but before the second is_dir (in the pattern matching), right? If that's the case having threads hammering the function may trigger it. I'm not sure that's solvable, it would return AlreadyExists when the directory was a directory and no longer exists.

dpc · 2017-02-14T18:10:21Z

@alexcrichton How to "lift it"? Move from librustc to libstd and then use that new location instead in librustc code (or just call directly from the old librustc name)?

@seppo0010 : I don't understand. If another thread called mkdir after current one did it, it would fail. Can you plese explain? I need more coffee. Anyway, I'd probably use the version @alexcrichton pointed to.

seppo0010 · 2017-02-14T18:45:18Z

@dpc sorry, my case is quite weird, let me write it down a more detailed version. Let's say process1 and process2 are calling create_dir_all with the same Path that's just a directory (no recursion) and then calling rmdir. The directory does not exist before this scenario.

process1: is_dir(path) # false
process2: is_dir(path) # false
process1: mkdir(path) # Ok(())
process2: mkdir(path) # Err(ErrorKind::AlreadyExists)
process1: rmdir # Ok(())
process2: is_dir(path) # false
process2 returns Err(ErrorKind::AlreadyExists)

I'm not saying it's a bug, but it might be unexpected, and I don't even know what may be better in that case.

alexcrichton · 2017-02-14T20:33:44Z

@dpc yeah we can just copy the code and then update the compiler to call libstd (as libstd would handle this race)

sfackler · 2017-02-28T21:49:31Z

Can this import get moved up to the top of the file with the other imports?

alexcrichton · 2017-03-07T23:37:07Z

ping @dpc, thoughts on updating this?

dpc · 2017-03-07T23:53:04Z

I agree with all feedback, and will get to it, probably on a weekend. If anyone would like you to pick it up, that's also fine with me.

alexcrichton · 2017-03-07T23:59:40Z

Thanks @dpc!

dpc · 2017-03-11T08:13:51Z

Ooops. Sorry. I wasn't able to compile anything on current master, but I was able to work with auto, and I pushed that.

Also, seems like create_dir_racy has a bug. If it was a file that was created, it won't detect it. It doesn't ever call path.is_dir... But it's late, so I'll double check everything during the weekend.

alexcrichton · 2017-03-12T20:30:41Z

+ Err(e) => return Err(e),
+ }
+ match path.parent() {
+ Some(p) => try!(create_dir_all(p)),


Could this call self.create_dir_all to ensure reuse of configuration in self? (if we add it in the future)

alexcrichton · 2017-03-12T20:31:15Z

Looking good to me, thanks! Could you also add a test for specifically this use case?

dpc · 2017-03-13T06:07:49Z

Done

alexcrichton · 2017-03-13T16:11:04Z

Looks good! I think Travis has a tidy error though

(r=me otherwise)

petrochenkov · 2017-03-13T17:05:10Z

@dpc
Maybe you could replace create_dir_racy with fs::create_dir_all in build_helper as well?

alexcrichton · 2017-03-13T17:10:08Z

@petrochenkov @dpc oh not yet, we'll need to wait for a new release. That's compiled against the stage0 libstd which doesn't have this fix.

alexcrichton · 2017-03-13T17:10:26Z

@bors: r+

bors · 2017-03-13T17:10:27Z

📌 Commit 4e2114f has been approved by alexcrichton

bors · 2017-03-13T18:39:43Z

⌛ Testing commit 4e2114f with merge e1be448...

bors · 2017-03-13T18:56:39Z

💔 Test failed - status-appveyor

alexcrichton · 2017-03-13T19:30:20Z

@bors: retry

spurious sccache failure Spurious failures in sccache #40240

bors · 2017-03-13T19:34:17Z

⌛ Testing commit 4e2114f with merge 4d01a7d...

Fix race condition in fs::create_dir_all 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.

frewsxcv · 2017-03-18T15:50:40Z

This seems to fail on Windows:

https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2436/job/ph8mhjt83f837xdl

@bors r-

dpc · 2017-03-19T03:04:04Z

I don't understand what happened, and I don't have Windows to debug. If one of the threads have failed, I would expect check! to print panic!("{} failed with: {}", stringify!($e), e), and I don't see it anywhere (and I did the last time).

Could someone with Windows help?

ollie27 · 2017-03-19T03:46:29Z


+ #[test]
+ fn concurrent_recursive_mkdir() {
+ for _ in 0..50 {


I think the main problem here is this use of tmpdir(). It deletes the path it returns when it goes out of scope and because it's not assigned to a variable that's immediately. See here. Changing this to the following should fix that:
let tmpdir = tmpdir();letmut dir = tmpdir.join("a");

ollie27 · 2017-03-19T03:54:17Z

 }

+ #[test]
+ fn concurrent_recursive_mkdir() {


Does this test really need to be run 50 times? It fails the first time for me on Windows.

It's because it relies on there being a race condition between the threads, but race conditions don't occur very often. By doing the test multiple times, it is much more likely to hit the race condition.

ollie27 · 2017-03-19T03:58:47Z

+ #[test]
+ fn concurrent_recursive_mkdir() {
+ for _ in 0..50 {
+ let mut dir = tmpdir().join("a");


Looks like you reduced the number of times this test is run rather than the the length of the path 😉.

Oh dear. 🤦‍

Increase lifetime of `tmpdir`, and really change the length of test path.

alexcrichton · 2017-03-19T15:35:44Z

@bors: r+

bors · 2017-03-19T15:35:45Z

📌 Commit 088696b has been approved by alexcrichton

bors · 2017-03-19T21:02:58Z

⌛ Testing commit 088696b with merge 6eb9960...

Fix race condition in fs::create_dir_all 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.

bors · 2017-03-19T23:50:01Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 6eb9960 to master...

emk · 2017-03-22T20:08:19Z

@dpc Thank you for fixing one of my least favorite Rust annoyances! The path to merge was a bit rock, but it's greatly appreciated.

dpc · 2017-03-22T21:31:30Z

@emk You're welcome. I'm very happy about it myself. First, I got on contributor's list. Second, it was a torn for me too. :)

This is a minor tweak to handle some problems which I've encountered several times now. The only real excuse for this change is that it's unobstrusive and has natural semantics. This patch modifies --filename to support two use cases: xsv partition --filename {}/cities.csv state . all-cities.csv xsv partition --filename {}/monuments.csv state . all-monuments.csv xsv partition --filename {}/parks.csv state . all-parks.csv Above, we want to partition our records by state into separate directories, but we have multiple kinds of data. xsv partition --filename {}/$(uuidgen).csv . input1.csv xsv partition --filename {}/$(uuidgen).csv . input2.csv Above, we're running multiple (possibly parallel) copies of xsv and we want to parition the data into multiple directories without a filename clash. There's one limitation to the implementation: We might theoretically hit the `create_dir_all` race condition recently fixed by rust-lang/rust#39799. I'm planning to supply a final patch which works around this race condition in stable Rust.

See rust-lang/rust#39799 for details.

rustdoc: Use `create_dir_all` to create output directory Currently rustdoc will fail if passed `-o foo/doc` if the `foo` directory doesn't exist. Also remove unneeded `mkdir` as `create_dir_all` can now handle concurrent invocations since #39799.

rustbuild: Replace create_dir_racy with create_dir_all `create_dir_all` has since been fixed (in #39799) so no need for `create_dir_racy`.

as std::fs::create_dir_all is now threadsafe rust-lang/rust#39799

rust-highfive assigned aturon Feb 14, 2017

frewsxcv added the T-libs-api [DEPRECATED; DO NOT USE] label Feb 14, 2017

sfackler reviewed Feb 28, 2017
View reviewed changes

Comment threadsrc/libstd/fs.rs Outdated
sfacklerFeb 28, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this import get moved up to the top of the file with the other imports?

dpc force-pushed the create_dir_all branch 2 times, most recently from bc9df80 to 9865187Compare March 11, 2017 07:58

dpc force-pushed the create_dir_all branch from c3a3480 to 07188feCompare March 11, 2017 17:13

alexcrichton reviewed Mar 12, 2017
View reviewed changes

frewsxcv mentioned this pull request Mar 18, 2017
Rollup of 10 pull requests #40629
Closed

frewsxcv mentioned this pull request Mar 18, 2017
Rollup of 8 pull requests #40633
Closed

ollie27 suggested changes Mar 19, 2017
View reviewed changes

Fix problems left in concurrent_recursive_mkdir
088696b
Increase lifetime of `tmpdir`, and really change the length of test path.

bors merged commit 088696b into rust-lang:masterMar 19, 2017

emk mentioned this pull request Mar 22, 2017
xsv partition subcommand BurntSushi/xsv#52
Merged
8 tasks

emk added a commit to faradayio/xsv that referenced this pull request Mar 23, 2017
Avoid create_dir_all race condition
d80b39c
See rust-lang/rust#39799 for details.

This was referenced Apr 4, 2017
More criteria and questions rust-lang/api-guidelines#11
Closed
One-syscall rule rust-lang/api-guidelines#18
Closed

dpc mentioned this pull request Apr 20, 2017
Too much useless io in chunk_write due to create_dir_all() dpc/rdedup#95
Closed

rocallahan mentioned this pull request May 17, 2017
Race condition in std::fs::create_dir_all. #33707
Closed

ollie27 mentioned this pull request Jun 9, 2017
rustdoc: Use create_dir_all to create output directory #42572
Merged

ollie27 mentioned this pull request Aug 7, 2017
rustbuild: Replace create_dir_racy with create_dir_all #43714
Merged

bors added a commit that referenced this pull request Aug 8, 2017
Auto merge of#43714- ollie27:rustbuild_create_dir_all, r=alexcrichton
c5e2051
rustbuild: Replace create_dir_racy with create_dir_all `create_dir_all` has since been fixed (in #39799) so no need for `create_dir_racy`.

stuhood mentioned this pull request Mar 22, 2023
Remove safe_create_dir(_all)pantsbuild/pants#18548
Closed

jqnatividad added a commit to dathere/qsv that referenced this pull request Nov 2, 2024
refactor: remove create_dir_all_threadsafe helper
d0af83b
as std::fs::create_dir_all is now threadsafe rust-lang/rust#39799

Uh oh!

Uh oh!

Conversation

dpc commented Feb 14, 2017

Uh oh!

rust-highfive commented Feb 14, 2017

Uh oh!

dpc commented Feb 14, 2017

Uh oh!

alexcrichton commented Feb 14, 2017

Uh oh!

seppo0010 commented Feb 14, 2017

Uh oh!

dpc commented Feb 14, 2017

Uh oh!

seppo0010 commented Feb 14, 2017

Uh oh!

alexcrichton commented Feb 14, 2017

Uh oh!

sfacklerFeb 28, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 7, 2017

Uh oh!

dpc commented Mar 7, 2017

Uh oh!

alexcrichton commented Mar 7, 2017

Uh oh!

dpc commented Mar 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichtonMar 12, 2017

Choose a reason for hiding this comment

Uh oh!

dpcMar 13, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 12, 2017

Uh oh!

dpc commented Mar 13, 2017

Uh oh!

alexcrichton commented Mar 13, 2017

Uh oh!

petrochenkov commented Mar 13, 2017

Uh oh!

alexcrichton commented Mar 13, 2017

Uh oh!

alexcrichton commented Mar 13, 2017

Uh oh!

bors commented Mar 13, 2017

Uh oh!

bors commented Mar 13, 2017

Uh oh!

bors commented Mar 13, 2017

Uh oh!

alexcrichton commented Mar 13, 2017

Uh oh!

bors commented Mar 13, 2017

Uh oh!

frewsxcv commented Mar 18, 2017

Uh oh!

dpc commented Mar 19, 2017

Uh oh!

ollie27Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

ollie27Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

retep998Mar 19, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ollie27Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

dpcMar 19, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 19, 2017

dpc commented Mar 11, 2017•
edited
Loading

retep998Mar 19, 2017•
edited
Loading