std: Modify TempDir to not fail on drop. Closes #12628

After discussion with Alex, we think the proper policy is for dtors
to not fail. This is consistent with C++. BufferedWriter already
does this, so this patch modifies TempDir to not fail in the dtor,
adding a `close` method for handling errors on destruction.

3 files changed, 106 insertions, 10 deletions

diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs
index a8e7b324bd7..68cbdd2e0aa 100644
--- a/src/libstd/io/buffered.rs
+++ b/src/libstd/io/buffered.rs
@@ -209,7 +209,7 @@ impl<W: Writer> Writer for BufferedWriter<W> {
 impl<W: Writer> Drop for BufferedWriter<W> {
     fn drop(&mut self) {
         if self.inner.is_some() {
-            // FIXME(#12628): should this error be ignored?
+            // dtors should not fail, so we ignore a failed flush
             let _ = self.flush_buf();
         }
     }
@@ -370,6 +370,7 @@ mod test {
     use io;
     use prelude::*;
     use super::*;
+    use super::super::{IoResult, EndOfFile};
     use super::super::mem::{MemReader, MemWriter, BufReader};
     use self::test::Bencher;
     use str::StrSlice;
@@ -584,6 +585,24 @@ mod test {
         assert_eq!(it.next(), None);
     }
 
+    #[test]
+    #[should_fail]
+    fn dont_fail_in_drop_on_failed_flush() {
+        struct FailFlushWriter;
+
+        impl Writer for FailFlushWriter {
+            fn write(&mut self, _buf: &[u8]) -> IoResult<()> { Ok(()) }
+            fn flush(&mut self) -> IoResult<()> { Err(io::standard_error(EndOfFile)) }
+        }
+
+        let writer = FailFlushWriter;
+        let _writer = BufferedWriter::new(writer);
+
+        // Trigger failure. If writer fails *again* due to the flush
+        // error then the process will abort.
+        fail!();
+    }
+
     #[bench]
     fn bench_buffered_reader(b: &mut Bencher) {
         b.iter(|| {
diff --git a/src/libstd/io/tempfile.rs b/src/libstd/io/tempfile.rs
index 8c28caa988a..b4fb95c8af7 100644
--- a/src/libstd/io/tempfile.rs
+++ b/src/libstd/io/tempfile.rs
@@ -10,7 +10,7 @@
 
 //! Temporary files and directories
 
-use io::fs;
+use io::{fs, IoResult};
 use io;
 use iter::{Iterator, range};
 use libc;
@@ -18,13 +18,14 @@ use ops::Drop;
 use option::{Option, None, Some};
 use os;
 use path::{Path, GenericPath};
-use result::{Ok, Err, ResultUnwrap};
+use result::{Ok, Err};
 use sync::atomics;
 
 /// A wrapper for a path to temporary directory implementing automatic
 /// scope-based deletion.
 pub struct TempDir {
-    path: Option<Path>
+    path: Option<Path>,
+    disarmed: bool
 }
 
 impl TempDir {
@@ -48,7 +49,7 @@ impl TempDir {
             let p = tmpdir.join(filename);
             match fs::mkdir(&p, io::UserRWX) {
                 Err(..) => {}
-                Ok(()) => return Some(TempDir { path: Some(p) })
+                Ok(()) => return Some(TempDir { path: Some(p), disarmed: false })
             }
         }
         None
@@ -75,15 +76,32 @@ impl TempDir {
     pub fn path<'a>(&'a self) -> &'a Path {
         self.path.get_ref()
     }
+
+    /// Close and remove the temporary directory
+    ///
+    /// Although `TempDir` removes the directory on drop, in the destructor
+    /// any errors are ignored. To detect errors cleaning up the temporary
+    /// directory, call `close` instead.
+    pub fn close(mut self) -> IoResult<()> {
+        self.cleanup_dir()
+    }
+
+    fn cleanup_dir(&mut self) -> IoResult<()> {
+        assert!(!self.disarmed);
+        self.disarmed = true;
+        match self.path {
+            Some(ref p) => {
+                fs::rmdir_recursive(p)
+            }
+            None => Ok(())
+        }
+    }
 }
 
 impl Drop for TempDir {
     fn drop(&mut self) {
-        for path in self.path.iter() {
-            if path.exists() {
-                // FIXME: is failing the right thing to do?
-                fs::rmdir_recursive(path).unwrap();
-            }
+        if !self.disarmed {
+            let _ = self.cleanup_dir();
         }
     }
 }
diff --git a/src/test/run-pass/tempfile.rs b/src/test/run-pass/tempfile.rs
index 437d6faea38..387a454542a 100644
--- a/src/test/run-pass/tempfile.rs
+++ b/src/test/run-pass/tempfile.rs
@@ -74,6 +74,50 @@ fn test_rm_tempdir() {
     assert!(!path.exists());
 }
 
+fn test_rm_tempdir_close() {
+    let (tx, rx) = channel();
+    let f: proc():Send = proc() {
+        let tmp = TempDir::new("test_rm_tempdir").unwrap();
+        tx.send(tmp.path().clone());
+        tmp.close();
+        fail!("fail to unwind past `tmp`");
+    };
+    task::try(f);
+    let path = rx.recv();
+    assert!(!path.exists());
+
+    let tmp = TempDir::new("test_rm_tempdir").unwrap();
+    let path = tmp.path().clone();
+    let f: proc():Send = proc() {
+        let tmp = tmp;
+        tmp.close();
+        fail!("fail to unwind past `tmp`");
+    };
+    task::try(f);
+    assert!(!path.exists());
+
+    let path;
+    {
+        let f = proc() {
+            TempDir::new("test_rm_tempdir").unwrap()
+        };
+        let tmp = task::try(f).ok().expect("test_rm_tmdir");
+        path = tmp.path().clone();
+        assert!(path.exists());
+        tmp.close();
+    }
+    assert!(!path.exists());
+
+    let path;
+    {
+        let tmp = TempDir::new("test_rm_tempdir").unwrap();
+        path = tmp.unwrap();
+    }
+    assert!(path.exists());
+    fs::rmdir_recursive(&path);
+    assert!(!path.exists());
+}
+
 // Ideally these would be in std::os but then core would need
 // to depend on std
 fn recursive_mkdir_rel() {
@@ -130,6 +174,19 @@ pub fn test_rmdir_recursive_ok() {
     assert!(!root.join("bar").join("blat").exists());
 }
 
+pub fn dont_double_fail() {
+    let r: Result<(), _> = task::try(proc() {
+        let tmpdir = TempDir::new("test").unwrap();
+        // Remove the temporary directory so that TempDir sees
+        // an error on drop
+        fs::rmdir(tmpdir.path());
+        // Trigger failure. If TempDir fails *again* due to the rmdir
+        // error then the process will abort.
+        fail!();
+    });
+    assert!(r.is_err());
+}
+
 fn in_tmpdir(f: ||) {
     let tmpdir = TempDir::new("test").expect("can't make tmpdir");
     assert!(os::change_dir(tmpdir.path()));
@@ -140,8 +197,10 @@ fn in_tmpdir(f: ||) {
 pub fn main() {
     in_tmpdir(test_tempdir);
     in_tmpdir(test_rm_tempdir);
+    in_tmpdir(test_rm_tempdir_close);
     in_tmpdir(recursive_mkdir_rel);
     in_tmpdir(recursive_mkdir_dot);
     in_tmpdir(recursive_mkdir_rel_2);
     in_tmpdir(test_rmdir_recursive_ok);
+    in_tmpdir(dont_double_fail);
 }