Merge tempdir api #33

KodrAus · 2017-09-12T03:57:39Z

Combines the tempdir API, beefs out the docs, and refactors NamedTempFile a bit.

If you'd rather I didn't refactor the internal structure then I can rejig this PR to minimise file movement.

Some previous discussion/iteration on combining the APIs: KodrAus#1

KodrAus · 2017-09-12T03:58:24Z

@Stebalien you'll probably want to take a thorough look through the docs I've added, I might've gotten some security notices wrong.

KodrAus · 2017-09-12T06:19:57Z

Support for 1.13.0 needs a quick change in remove_dir_all

Stebalien · 2017-09-12T20:45:59Z

I will review this ASAP but that probably won't be till the weekend.

KodrAus · 2017-09-22T11:50:36Z

Friendly ping @Stebalien to take a look at this when you've got bandwidth. No pressure though.

We could move forward without that fix in remove_dir_all but it would mean bumping the minimum rust version supported.

opilar · 2017-09-23T21:38:15Z

A little question: when tempdir and tempfile will merge apis, should we rename the crate to temp?

KodrAus · 2017-09-24T00:45:34Z

@opilar the idea is to keep the tempfile crate name.

Stebalien · 2017-09-27T16:22:57Z

@KodrAus sorry, my family ended up visiting that weekend and now I'm traveling but I'll try to get to this in the next few days.

Stebalien · 2017-10-10T20:39:22Z

I'm still alive, FYI (just busy). Sorry.

KodrAus · 2017-10-10T20:43:16Z

@Stebalien that's no problem :) We're not working on a tight timeline here so there's no hurry, no pressure. I just remembered there was a new version of remove_dir_all I could use.

azriel91 · 2017-10-21T06:16:47Z

ping! I came across this when looking to write tests that use a temporary directory. Is this PR close to being merged, or shall I use tempdir?

KodrAus · 2017-10-30T01:42:51Z

@azriel91 sorry I just got a chance to loop back here. I would use tempdir. We'll have a nice smooth upgrade path when the APIs are merged, but this PR is only one step of that process.

Stebalien

Wow. A review? Yes! A review! Sorry, the line change count turned me off reviewing this but it really didn't take that long (lots of nice documentation). This looks really good.

Three main things:

Misc doc fixes.
Make tempdir functions consistent with tempfile functions.
Reuse the builder and generalize it to directories (importantly, reusing the name logic).

Also, please rebase or allow me to push a rebased version. There are now a few merge conflicts.

At this point, I'd totally understand if you've moved on and can't work on this. If so, I can take this to completion.

Stebalien · 2018-01-08T02:14:35Z

src/lib.rs

-//! its destructor doesn't run. This is because `tempfile()` relies on the OS to cleanup the
-//! underlying file so the file while `NamedTempFile` relies on its destructor to do so.
+//! `tempfile` will (almost) never fail to cleanup temporary resources but `TempDir` and `NamedTempFile` will if
+//! their destructor doesn't run. This is because `tempfile` relies on the OS to cleanup the


distructors

Stebalien · 2018-01-08T02:15:11Z

src/lib.rs

-//! underlying file so the file while `NamedTempFile` relies on its destructor to do so.
+//! `tempfile` will (almost) never fail to cleanup temporary resources but `TempDir` and `NamedTempFile` will if
+//! their destructor doesn't run. This is because `tempfile` relies on the OS to cleanup the
+//! underlying file so the file while `TempDir` and `NamedTempFile` rely on their destructor to do so.


destructors

Stebalien · 2018-01-08T02:16:30Z

src/lib.rs

+
+#![doc(html_logo_url = "https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png",
+       html_favicon_url = "https://www.rust-lang.org/favicon.ico",
+       html_root_url = "https://docs.rs/tempdir/0.3.5")]


should probably remove this (or at least replace tempdir with tempfile).

Stebalien · 2018-01-08T02:40:00Z

src/dir.rs

+///
+/// [`TempDir`]: struct.TempDir.html
+/// [resource-leaking]: struct.TempDir.html#resource-leaking
+pub fn tempdir(prefix: &str) -> io::Result<TempDir> {


I'd prefer it if this used the same temporary file builder system as NamedTempFile. As a matter of fact, we can even reuse the same builder (if we generalize it). I'd change the create and create_in methods on the builder to file and file_in and add directory and directory_in methods.

Stebalien · 2018-01-08T02:40:37Z

src/dir.rs

+    /// # Ok(())
+    /// # }
+    /// ```
+    pub fn new(prefix: &str) -> io::Result<TempDir> {


Same here (I wouldn't include the prefix option).

Stebalien · 2018-01-08T02:40:48Z

src/dir.rs

+    /// # Ok(())
+    /// # }
+    /// ```
+    pub fn new_in<P: AsRef<Path>>(tmpdir: P, prefix: &str) -> io::Result<TempDir> {


Same (no prefix option, use the builder). That should reduce code duplication.

Stebalien · 2018-01-08T02:42:55Z

src/dir.rs

+            let cur_dir = env::current_dir()?;
+            storage = cur_dir.join(tmpdir);
+            tmpdir = &storage;
+            // return TempDir::new_in(&cur_dir.join(tmpdir), prefix);


commented out code?

Stebalien · 2018-01-08T03:04:48Z

src/file/mod.rs

+        &mut self.inner_mut().file
+    }
+
+    /// Convert the temporary file into a `std::fs::File`.


This should note that the inner file will be deleted.

KodrAus · 2018-01-08T03:39:44Z

Thanks for the thorough review @Stebalien! I'll rebase and work through the changes over the next week 👍

KodrAus · 2018-01-15T22:49:01Z

I've pushed up some changes based on the feedback because having code only living on my local machine makes me nervous...

I haven't updated the doc tests yet, and the behaviour of TempDir is now subtly different with respect to leading .s in the directory name. I think it would be good to think a bit about what we might want to do with the Builder in the future, and whether we're painting ourselves into any corners by sharing it between temporary files and directories.

Stebalien · 2018-01-18T21:27:56Z

I'll try to take a look at it this weekend (and not just put this off for a few months this time...).

Note: I'm not entirely against changing the default for tempfile if it's a problem for tempdir. IIRC, we never made any guarantees (unless the user explicitly specified the prefix).

KodrAus · 2018-01-18T22:28:31Z

No problem! I'll try get the tests and docs updated before then.

jasonwhite

I took it upon myself to review this. I'd like to get this merged because my PR is blocked on this one.

I noticed some of the new files have trailing white space. Would be nice to remove that. (Sorry, I'm a little pedantic about this because my editor shows it in bright red.)

jasonwhite · 2018-01-26T06:10:09Z

src/dir.rs

+/// receiving a signal like `SIGINT`, then the temporary directory
+/// will not be deleted.
+/// 
+// # Examples


Missing a triple slash (///) here.

jasonwhite · 2018-01-26T06:24:36Z

src/dir.rs

+        let result = remove_dir_all(self.path());
+
+        // Prevent the Drop impl from removing the dir a second time.
+        self.path = None;


If the entire reason for wrapping the path in an Option is to prevent Drop from attempting to delete the directory a second time, then you should be able to eliminate the need for Option with this:

pub fn close(mut self) -> io::Result<()> { // Replace the path with an empty one to avoid a memory leak by mem::forget. let path = std::mem::replace(self.path, PathBuf::new()); std::mem::forget(self); // Don't call drop remove_dir_all(&path) }

The same would have to be done to into_path() of course.

This is cleaner than using an Option in my opinion.

Another problem to consider is what to do if close() fails. The caller might wish to implement a retry if, for example, they've created a temporary directory on a network file system that might have intermittent failures. (I encounter this problem a lot with network shares under Windows.)

However, it looks like you want API compatibility with the tempdir crate, so you probably can't solve this problem.

Edit: It looks like this problem has already been talked to death in rust-lang/api-guidelines#61. Unfortunately, it seems the conclusion is that the failure should only be reported and not handled by the program. Retries of file operations on spotty Windows network shares have solved a lot of problems for me in the past.

The caller might wish to implement a retry if, for example, they've created a temporary directory on a network file system that might have intermittent failures.

...

It looks like this problem has already been talked to death in

Also, in posix, one should generally not retry a close. The correct way to handle such errors is to catch them with an fsync first.

@Stebalien Are you talking about close(2) or remove_dir_all? I agree about close(2), but deleting files over a network can have intermittent failures where retrying is the right thing to do.

Ah. I was thinking about close. You're right, this is a bit different. However, I kind of feel that this is, really, the best API. We could put the path into the error but that'll make this less ergonomic.

jasonwhite · 2018-01-26T06:27:52Z

Cargo.toml

-version = "2.2.0"
-authors = ["Steven Allen <steven@stebalien.com>"]
-description = "Securely create temporary files."
+version = "2.1.6"


Looks like a package version number regression here.

Ah well spotted!

jasonwhite · 2018-01-26T06:29:20Z

src/lib.rs


+#![doc(html_logo_url = "https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png",
+       html_favicon_url = "https://www.rust-lang.org/favicon.ico",
+       html_root_url = "https://docs.rs/tempfile/2.1.6")]


This version number should match the one in Cargo.toml (once the one in there is fixed).

Stebalien

Thanks, nice job with the builder. However, I think I may have given you the wrong impression in a few of my comments regarding the helper functions.

Stebalien · 2018-01-27T01:29:48Z

src/lib.rs

+    /// ```
+    ///
+    /// [`NamedTempFile::new`]: struct.NamedTempFile.html#method.new
+    pub fn named_tempfile(&self) -> io::Result<NamedTempFile> {


Kind of a nit... this is a really long name. I'd rather just tempfile or, even better, file and directory (the fact that they're temporary is, IMO, unambiguous).

That's a fair point 👍

Stebalien · 2018-01-27T01:36:05Z

src/lib.rs

+            dir = &storage;
+        }
+
+        for _ in 0..::NUM_RETRIES {


It would be nice to refactor this out into a helper function if possible.

fn create_helper<T, F: Fn(PathBuf) -> io::Result<T>>(base: &Path, f: F) -> io::Result<T> { /* ... */ } // ... create_helper(path, dir::create)

Stebalien · 2018-01-27T01:37:59Z

src/dir.rs

-
-        Err(Error::new(ErrorKind::AlreadyExists,
-                       "too many temporary directories already exist"))
+    pub fn new() -> io::Result<TempDir> {


I think there may have been a miscommunication here. I still think providing a new_in is worth it for the convenience, I just wanted it to use the builder internally (code duplication) and have neither new nor new_in take a prefix argument (mirror NamedTempFile).

Ah yes, I did misunderstand, I'll restore those methods 👍

Stebalien · 2018-01-27T01:38:40Z

src/file/mod.rs

-    /// ```
-    ///
-    /// [`NamedTempFile::new`]: #method.new
-    pub fn new_in<P: AsRef<Path>>(dir: P) -> io::Result<NamedTempFile> {


Same here, I'd keep the new_in.

Stebalien · 2018-01-27T01:39:51Z

src/dir.rs

@@ -355,3 +233,8 @@ impl Drop for TempDir {
        }
    }
 }
+
+// pub(crate)
+pub fn create(path: PathBuf) -> io::Result<TempDir> {


What was wrong with tempdir, tempdir_in, named_tempfile, and named_tempfile_in? This may also have been a miscommunication on my part. I meant that one should use the builder when specifying custom options, not that we shouldn't have these convenience methods (they are quite convenient).

Yep, that was another misunderstanding, I'll restore these too.

Stebalien · 2018-01-27T01:42:47Z

src/dir.rs

+    }
+}
+
+// pub(crate)


This isn't actually pub(crate). This module isn't exported (as far as I can tell).

That's right, it's just a note that this method is accessible throughout the crate, but not public since our min Rust version doesn't support pub(restricted).

Stebalien · 2018-01-27T02:37:54Z

src/dir.rs

+        let result = remove_dir_all(self.path());
+
+        // Prevent the Drop impl from removing the dir a second time.
+        self.path = None;


Ah. I was thinking about close. You're right, this is a bit different. However, I kind of feel that this is, really, the best API. We could put the path into the error but that'll make this less ergonomic.

KodrAus · 2018-01-27T03:42:11Z

Thanks @jasonwhite and @Stebalien! I'll work through this round of feedback over this week.

Stebalien · 2018-01-27T07:05:00Z

Awesome! Thanks for putting this together.

KodrAus · 2018-02-06T05:37:55Z

@Stebalien @jasonwhite this should be ready for another look now.

jasonwhite

Looks good to me!

Stebalien · 2018-02-07T02:53:50Z

🎉

@KodrAus thanks so much for both implementing this and then bearing with me on this long and drawn out CR.

@jasonwhite thanks for pitching in and waiting for this CR to complete.

KodrAus · 2018-02-07T03:02:49Z

Thanks all!

I'll get the ball rolling on deprecating the tempdir crate. Looks like we've got an open design question in tempdir we might want to look at here but otherwise everything looks good on that end.

KodrAus force-pushed the feat/tempdir branch from 4f8a283 to 118e964 Compare October 10, 2017 20:21

KodrAus mentioned this pull request Jan 4, 2018

bump rand to 0.4 #34

Merged

Stebalien force-pushed the feat/tempdir branch from 118e964 to 7b15ad6 Compare January 8, 2018 02:30

Stebalien force-pushed the master branch from 99f289a to cbfdbbd Compare January 8, 2018 02:34

Stebalien requested changes Jan 8, 2018

View reviewed changes

This was referenced Jan 10, 2018

Fix warning about unused variable #37

Closed

Close NamedTempFile without deleting #36

Closed

jasonwhite mentioned this pull request Jan 11, 2018

Allow closing of a NamedTempFile file handle #38

Merged

merge tempdir api

328f648

KodrAus force-pushed the feat/tempdir branch from 7b15ad6 to 328f648 Compare January 14, 2018 23:30

use same builder for temp files and dirs

aafb469

KodrAus added 2 commits January 21, 2018 08:58

fix up unit tests

8496b9e

fix up docs

020c8a8

jasonwhite suggested changes Jan 26, 2018

View reviewed changes

Stebalien requested changes Jan 27, 2018

View reviewed changes

restore convenience functions

bde363d

jasonwhite approved these changes Feb 6, 2018

View reviewed changes

Stebalien approved these changes Feb 7, 2018

View reviewed changes

Stebalien merged commit 30d74f7 into Stebalien:master Feb 7, 2018

JohnTitor mentioned this pull request Jan 1, 2020

Use tempfile instead of tempdir tokio-rs/mio#1237

Closed

Merge tempdir api #33

Merge tempdir api #33

Conversation

KodrAus commented Sep 12, 2017

KodrAus commented Sep 12, 2017

KodrAus commented Sep 12, 2017

Stebalien commented Sep 12, 2017

KodrAus commented Sep 22, 2017

opilar commented Sep 23, 2017

KodrAus commented Sep 24, 2017

Stebalien commented Sep 27, 2017

Stebalien commented Oct 10, 2017

KodrAus commented Oct 10, 2017

azriel91 commented Oct 21, 2017

KodrAus commented Oct 30, 2017

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented Jan 8, 2018

KodrAus commented Jan 15, 2018

Stebalien commented Jan 18, 2018 • edited Loading

KodrAus commented Jan 18, 2018

jasonwhite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonwhite Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented Jan 27, 2018

Stebalien commented Jan 27, 2018

KodrAus commented Feb 6, 2018

jasonwhite left a comment

Choose a reason for hiding this comment

Stebalien commented Feb 7, 2018

KodrAus commented Feb 7, 2018

Stebalien commented Jan 18, 2018 •

edited

Loading

jasonwhite Jan 26, 2018 •

edited

Loading