Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update remove_all_dirs to 0.7 #149

Closed
wants to merge 1 commit into from

Conversation

jbtrystram
Copy link

Bump remove all dirs to a more recent version

@Stebalien
Copy link
Owner

See #129 (sorry, should have left a comment here earlier). I don't like depending on older versions, but I definitely don't want a temporary file library spinning off threads, depending on rayon, etc.

@Stebalien Stebalien closed this May 18, 2021
@jbtrystram
Copy link
Author

Thanks for the explanation

@apiraino
Copy link

apiraino commented May 31, 2021

hi @Stebalien I'd love to learn more on this topic.

I notice the remove_dir_all crate maintainers declare that their crate behaves so that "on non-Windows this is a re-export of std::fs::remove_dir_all" and only on Windows target the rayon dependency adds concurrent files deletion. I think this is their line of thought on why are using rayon. Towards the end they mention a possible race condition on concurrent files deletion.

I'd be happy if you could share your thoughts on why using a concurrent approach is not great. I'm not questioning your choice, just interested in more technical details.

If remove_dir_all=0.5 is feature-complete in the context of the tempfile crate and there are no other impellent reasons to push for an upgrade, as a user I'm fine with this crate holding the dependency upgrade, otherwise did you evaluate if this warrants dropping remove_dir_all for something else?

Would it be possible to use two versions of the remove_dir_all dependency? Let's say 0.5 on Windows and the latest on all the other platforms, example:

[target.'cfg(any(unix))'.dependencies]
remove_dir_all = "0.7"
[target.'cfg(any(windows))'.dependencies]
remove_dir_all = "0.5"

Would that be a nightmare from the maintenaince point of view?

Final question (perhaps related): is this the reason why tempfile stays synchronous and does not leverage the Tokio/Async-std runtime for async operations?

thank you so much :-)

@Stebalien
Copy link
Owner

I'd be happy if you could share your thoughts on why using a concurrent approach is not great. I'm not questioning your choice, just interested in more technical details.

  1. It's a large, complex dependency tree. Ideally adding tempfile to your project shouldn't add too much weight.
  2. The principle of least surprise. I'd be very surprised if a temporary file library decided to spin up a thread pool on drop. I'd likely make safety assumptions assuming this can't happen.

I'd be happy if you could share your thoughts on why using a concurrent approach is not great. I'm not questioning your choice, just interested in more technical details.

Unfortunately, I'm not aware of any reasonable alternatives at the moment. I'd love to upgrade, but I'd need two things:

  1. A feature to disable pulling in the rayon dependency (issue 1).
  2. A variant of remove_dir_all that doesn't spin up a thread pool.

With that, I'd be happy to add a parallel_delete option to the tempdir builder to actually make use of the parallel deletion feature. I just want it to be opt-in.

Would it be possible to use two versions of the remove_dir_all dependency?

Does that really buy us anything? That seems like it's more likely to cause confusion than actually solve anything.

Final question (perhaps related): is this the reason why tempfile stays synchronous and does not leverage the Tokio/Async-std runtime for async operations?

No, I just haven't looked into it yet (and there's no async drop so we're a bit limited).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants