From db00ba9eb2afa1a3db24e208cbd63125d0157090 Mon Sep 17 00:00:00 2001 From: David Roundy Date: Tue, 1 Dec 2015 17:29:56 -0500 Subject: 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/libstd/fs.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index b074e8b86b9..bb8b4064fab 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) } } -- cgit 1.4.1-3-g733a5 From 0ec28b796d1206c4442f0269febe2a1cc0794411 Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Sat, 11 Mar 2017 08:04:30 -0800 Subject: 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(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index bb8b4064fab..6a465e38f38 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), } } -- cgit 1.4.1-3-g733a5 From f2adee74a318503334c07290ca767fd609aebf1f Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Sun, 12 Mar 2017 23:07:16 -0700 Subject: Add `concurrent_recursive_mkdir` test --- src/libstd/fs.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 6a465e38f38..7ba88d6a3e1 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("/"))); -- cgit 1.4.1-3-g733a5 From a51c6aaf848a6cd64ace890effb6e377bbefce7d Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Mon, 13 Mar 2017 22:29:00 -0700 Subject: Break line longer than 100 characters --- src/libstd/fs.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 7ba88d6a3e1..e26b84d6b07 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), } -- cgit 1.4.1-3-g733a5 From c3e2eaf4cb774f776f4022406742f978580c0a53 Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Tue, 14 Mar 2017 22:29:00 -0700 Subject: 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(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index e26b84d6b07..91cf0d44d9d 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"); -- cgit 1.4.1-3-g733a5 From bcae6a3734cdcecffc3ee30981a5b9107bee2ee0 Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Tue, 14 Mar 2017 22:29:00 -0700 Subject: 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(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 91cf0d44d9d..59a6a99201f 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() { -- cgit 1.4.1-3-g733a5 From b5d590b3f0483bc943df8f59011b85500456d748 Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Tue, 14 Mar 2017 22:03:00 -0700 Subject: Fix `create_dir_all("")` Add a test for `""` and `"."`. --- src/libstd/fs.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 59a6a99201f..cc9df743ef1 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(); -- cgit 1.4.1-3-g733a5 From 088696b98fca3c38f109ef97e17bd5403212b10c Mon Sep 17 00:00:00 2001 From: Dawid Ciężarkiewicz Date: Sat, 18 Mar 2017 21:03:51 -0700 Subject: 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(-) (limited to 'src/libstd') diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index cc9df743ef1..ca26dc9527c 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!(); -- cgit 1.4.1-3-g733a5