From db00ba9eb2afa1a3db24e208cbd63125d0157090 Mon Sep 17 00:00:00 2001 From: David Roundy Date: Tue, 1 Dec 2015 17:29:56 -0500 Subject: [PATCH 1/8] Fix race condition in fs::create_dir_all It is more robust to not fail if any directory in a path was created concurrently. This change lifts rustc internal `create_dir_racy` that was created to handle such conditions to be new `create_dir_all` implementation. --- src/librustc/util/fs.rs | 20 ------------------ src/librustc_incremental/persist/fs.rs | 2 +- src/librustc_save_analysis/lib.rs | 2 +- src/libstd/fs.rs | 24 ++++++++++++++++++---- src/tools/compiletest/src/runtest.rs | 28 ++++++-------------------- 5 files changed, 28 insertions(+), 48 deletions(-) diff --git a/src/librustc/util/fs.rs b/src/librustc/util/fs.rs index da6a202e5afb2..090753b18c0ba 100644 --- a/src/librustc/util/fs.rs +++ b/src/librustc/util/fs.rs @@ -109,23 +109,3 @@ pub fn rename_or_copy_remove, Q: AsRef>(p: P, } } } - -// Like std::fs::create_dir_all, except handles concurrent calls among multiple -// threads or processes. -pub fn create_dir_racy(path: &Path) -> io::Result<()> { - match fs::create_dir(path) { - Ok(()) => return Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => return Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::NotFound => (), - Err(e) => return Err(e), - } - match path.parent() { - Some(p) => try!(create_dir_racy(p)), - None => return Err(io::Error::new(io::ErrorKind::Other, "failed to create whole tree")), - } - match fs::create_dir(path) { - Ok(()) => Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => Ok(()), - Err(e) => Err(e), - } -} diff --git a/src/librustc_incremental/persist/fs.rs b/src/librustc_incremental/persist/fs.rs index 2ad37e98c708a..2a4a01cd4a453 100644 --- a/src/librustc_incremental/persist/fs.rs +++ b/src/librustc_incremental/persist/fs.rs @@ -461,7 +461,7 @@ fn generate_session_dir_path(crate_dir: &Path) -> PathBuf { } fn create_dir(sess: &Session, path: &Path, dir_tag: &str) -> Result<(),()> { - match fs_util::create_dir_racy(path) { + match std_fs::create_dir_all(path) { Ok(()) => { debug!("{} directory created successfully", dir_tag); Ok(()) diff --git a/src/librustc_save_analysis/lib.rs b/src/librustc_save_analysis/lib.rs index 111c8370be2b1..1cc73a3dce045 100644 --- a/src/librustc_save_analysis/lib.rs +++ b/src/librustc_save_analysis/lib.rs @@ -885,7 +885,7 @@ pub fn process_crate<'l, 'tcx>(tcx: TyCtxt<'l, 'tcx, 'tcx>, }, }; - if let Err(e) = rustc::util::fs::create_dir_racy(&root_path) { + if let Err(e) = std::fs::create_dir_all(&root_path) { tcx.sess.err(&format!("Could not create directory {}: {}", root_path.display(), e)); diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index b074e8b86b9a3..bb8b4064fab7a 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1534,6 +1534,12 @@ pub fn create_dir>(path: P) -> io::Result<()> { /// error conditions for when a directory is being created (after it is /// determined to not exist) are outlined by `fs::create_dir`. /// +/// Notable exception is made for situations where any of the directories +/// specified in the `path` could not be created as it was created concurrently. +/// Such cases are considered success. In other words: calling `create_dir_all` +/// concurrently from multiple threads or processes is guaranteed to not fail +/// due to race itself. +/// /// # Examples /// /// ``` @@ -1769,11 +1775,21 @@ impl DirBuilder { } fn create_dir_all(&self, path: &Path) -> io::Result<()> { - if path == Path::new("") || path.is_dir() { return Ok(()) } - if let Some(p) = path.parent() { - self.create_dir_all(p)? + match self.inner.mkdir(path) { + Ok(()) => return Ok(()), + Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => return Ok(()), + Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} + Err(e) => return Err(e), + } + match path.parent() { + Some(p) => try!(create_dir_all(p)), + None => return Err(io::Error::new(io::ErrorKind::Other, "failed to create whole tree")), + } + match self.inner.mkdir(path) { + Ok(()) => Ok(()), + Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => Ok(()), + Err(e) => Err(e), } - self.inner.mkdir(path) } } diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 1ec0838d45f76..2865fa6a79253 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -25,7 +25,7 @@ use util::logv; use std::collections::HashSet; use std::env; use std::fmt; -use std::fs::{self, File}; +use std::fs::{self, File, create_dir_all}; use std::io::prelude::*; use std::io::{self, BufReader}; use std::path::{Path, PathBuf}; @@ -395,7 +395,7 @@ actual:\n\ let out_dir = self.output_base_name().with_extension("pretty-out"); let _ = fs::remove_dir_all(&out_dir); - self.create_dir_racy(&out_dir); + create_dir_all(&out_dir).unwrap(); // FIXME (#9639): This needs to handle non-utf8 paths let mut args = vec!["-".to_owned(), @@ -1269,7 +1269,7 @@ actual:\n\ fn compose_and_run_compiler(&self, args: ProcArgs, input: Option) -> ProcRes { if !self.props.aux_builds.is_empty() { - self.create_dir_racy(&self.aux_output_dir_name()); + create_dir_all(&self.aux_output_dir_name()).unwrap(); } let aux_dir = self.aux_output_dir_name(); @@ -1340,22 +1340,6 @@ actual:\n\ input) } - // Like std::fs::create_dir_all, except handles concurrent calls among multiple - // threads or processes. - fn create_dir_racy(&self, path: &Path) { - match fs::create_dir(path) { - Ok(()) => return, - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => return, - Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} - Err(e) => panic!("failed to create dir {:?}: {}", path, e), - } - self.create_dir_racy(path.parent().unwrap()); - match fs::create_dir(path) { - Ok(()) => {} - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => {} - Err(e) => panic!("failed to create dir {:?}: {}", path, e), - } - } fn compose_and_run(&self, ProcArgs{ args, prog }: ProcArgs, @@ -1435,7 +1419,7 @@ actual:\n\ let mir_dump_dir = self.get_mir_dump_dir(); - self.create_dir_racy(mir_dump_dir.as_path()); + create_dir_all(mir_dump_dir.as_path()).unwrap(); let mut dir_opt = "dump-mir-dir=".to_string(); dir_opt.push_str(mir_dump_dir.to_str().unwrap()); debug!("dir_opt: {:?}", dir_opt); @@ -1923,7 +1907,7 @@ actual:\n\ let out_dir = self.output_base_name(); let _ = fs::remove_dir_all(&out_dir); - self.create_dir_racy(&out_dir); + create_dir_all(&out_dir).unwrap(); let proc_res = self.document(&out_dir); if !proc_res.status.success() { @@ -2299,7 +2283,7 @@ actual:\n\ if tmpdir.exists() { self.aggressive_rm_rf(&tmpdir).unwrap(); } - self.create_dir_racy(&tmpdir); + create_dir_all(&tmpdir).unwrap(); let host = &self.config.host; let make = if host.contains("bitrig") || host.contains("dragonfly") || From 0ec28b796d1206c4442f0269febe2a1cc0794411 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Sat, 11 Mar 2017 08:04:30 -0800 Subject: [PATCH 2/8] Fix new version of `create_dir_all` It will now correctly fail on existing non-directories. --- src/libstd/fs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index bb8b4064fab7a..6a465e38f3810 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1777,7 +1777,7 @@ impl DirBuilder { fn create_dir_all(&self, path: &Path) -> io::Result<()> { match self.inner.mkdir(path) { Ok(()) => return Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => return Ok(()), + Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => return Ok(()), Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} Err(e) => return Err(e), } @@ -1787,7 +1787,7 @@ impl DirBuilder { } match self.inner.mkdir(path) { Ok(()) => Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => Ok(()), + Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => Ok(()), Err(e) => Err(e), } } From f2adee74a318503334c07290ca767fd609aebf1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Sun, 12 Mar 2017 23:07:16 -0700 Subject: [PATCH 3/8] Add `concurrent_recursive_mkdir` test --- src/libstd/fs.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 6a465e38f3810..7ba88d6a3e13c 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1782,7 +1782,7 @@ impl DirBuilder { Err(e) => return Err(e), } match path.parent() { - Some(p) => try!(create_dir_all(p)), + Some(p) => try!(self.create_dir_all(p)), None => return Err(io::Error::new(io::ErrorKind::Other, "failed to create whole tree")), } match self.inner.mkdir(path) { @@ -1809,6 +1809,7 @@ mod tests { use rand::{StdRng, Rng}; use str; use sys_common::io::test::{TempDir, tmpdir}; + use thread; #[cfg(windows)] use os::windows::fs::{symlink_dir, symlink_file}; @@ -2276,6 +2277,26 @@ mod tests { assert!(result.is_err()); } + #[test] + fn concurrent_recursive_mkdir() { + for _ in 0..100 { + let mut dir = tmpdir().join("a"); + for _ in 0..100 { + dir = dir.join("a"); + } + let mut join = vec!(); + for _ in 0..8 { + let dir = dir.clone(); + join.push(thread::spawn(move || { + check!(fs::create_dir_all(&dir)); + })) + } + + // No `Display` on result of `join()` + join.drain(..).map(|join| join.join().unwrap()).count(); + } + } + #[test] fn recursive_mkdir_slash() { check!(fs::create_dir_all(&Path::new("/"))); From a51c6aaf848a6cd64ace890effb6e377bbefce7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Mon, 13 Mar 2017 22:29:00 -0700 Subject: [PATCH 4/8] Break line longer than 100 characters --- src/libstd/fs.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 7ba88d6a3e13c..e26b84d6b0786 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1777,7 +1777,8 @@ impl DirBuilder { fn create_dir_all(&self, path: &Path) -> io::Result<()> { match self.inner.mkdir(path) { Ok(()) => return Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => return Ok(()), + Err(ref e) + if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => return Ok(()), Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} Err(e) => return Err(e), } From c3e2eaf4cb774f776f4022406742f978580c0a53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Tue, 14 Mar 2017 22:29:00 -0700 Subject: [PATCH 5/8] Fix problems found on Windows in `dir_create_all` Ignore the type of error altogether. The rationale is: it doesn't matter what was the problem if the directory is there. In the previous versions if the directory was there already we wouldn't even attempt to create it, so we wouldn't know about the problem neither. Make test path length smaller in `concurrent_recursive_mkdir` test. --- src/libstd/fs.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index e26b84d6b0786..91cf0d44d9d8b 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1777,8 +1777,7 @@ impl DirBuilder { fn create_dir_all(&self, path: &Path) -> io::Result<()> { match self.inner.mkdir(path) { Ok(()) => return Ok(()), - Err(ref e) - if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => return Ok(()), + Err(_) if path.is_dir() => return Ok(()), Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} Err(e) => return Err(e), } @@ -1788,7 +1787,7 @@ impl DirBuilder { } match self.inner.mkdir(path) { Ok(()) => Ok(()), - Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => Ok(()), + Err(_) if path.is_dir() => Ok(()), Err(e) => Err(e), } } @@ -2280,7 +2279,7 @@ mod tests { #[test] fn concurrent_recursive_mkdir() { - for _ in 0..100 { + for _ in 0..50 { let mut dir = tmpdir().join("a"); for _ in 0..100 { dir = dir.join("a"); From bcae6a3734cdcecffc3ee30981a5b9107bee2ee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Tue, 14 Mar 2017 22:29:00 -0700 Subject: [PATCH 6/8] Reorder match checks in `create_dir_all` Avoid doing `is_dir` in the fast path. --- src/libstd/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 91cf0d44d9d8b..59a6a99201f1e 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1777,8 +1777,8 @@ impl DirBuilder { fn create_dir_all(&self, path: &Path) -> io::Result<()> { match self.inner.mkdir(path) { Ok(()) => return Ok(()), - Err(_) if path.is_dir() => return Ok(()), Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} + Err(_) if path.is_dir() => return Ok(()), Err(e) => return Err(e), } match path.parent() { From b5d590b3f0483bc943df8f59011b85500456d748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Tue, 14 Mar 2017 22:03:00 -0700 Subject: [PATCH 7/8] Fix `create_dir_all("")` Add a test for `""` and `"."`. --- src/libstd/fs.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 59a6a99201f1e..cc9df743ef1da 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1775,6 +1775,10 @@ impl DirBuilder { } fn create_dir_all(&self, path: &Path) -> io::Result<()> { + if path == Path::new("") { + return Ok(()) + } + match self.inner.mkdir(path) { Ok(()) => return Ok(()), Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} @@ -2302,6 +2306,16 @@ mod tests { check!(fs::create_dir_all(&Path::new("/"))); } + #[test] + fn recursive_mkdir_dot() { + check!(fs::create_dir_all(&Path::new("."))); + } + + #[test] + fn recursive_mkdir_empty() { + check!(fs::create_dir_all(&Path::new(""))); + } + #[test] fn recursive_rmdir() { let tmpdir = tmpdir(); From 088696b98fca3c38f109ef97e17bd5403212b10c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Sat, 18 Mar 2017 21:03:51 -0700 Subject: [PATCH 8/8] Fix problems left in `concurrent_recursive_mkdir` Increase lifetime of `tmpdir`, and really change the length of test path. --- src/libstd/fs.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index cc9df743ef1da..ca26dc9527c04 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -2283,9 +2283,10 @@ mod tests { #[test] fn concurrent_recursive_mkdir() { - for _ in 0..50 { - let mut dir = tmpdir().join("a"); - for _ in 0..100 { + for _ in 0..100 { + let dir = tmpdir(); + let mut dir = dir.join("a"); + for _ in 0..40 { dir = dir.join("a"); } let mut join = vec!();