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

docs(std): clarify remove_dir_all errors #105745

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Conversation

philpax
Copy link
Contributor

@philpax philpax commented Dec 15, 2022

When using remove_dir_all, I assumed that the function was idempotent and that I could always call it to remove a directory if it existed. That's not the case and it bit me in production, so I figured I'd submit this to clarify the docs.

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 15, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rbtcollins
Copy link
Contributor

I'm a bit conflicted here. There are a number of other error cases documented at the linked functions. e.g. not being a directory, not having permissions to delete it and so on.

Perhaps instead of that specific a note it should say something like 'remove_dir_all will fail if remove_dir or remove_file fail on any of the paths including the root of the three being deleted?

@philpax
Copy link
Contributor Author

philpax commented Jan 22, 2023

Hm, that seems reasonable, but I'd want to draw particular attention to it failing if you try to delete a non-existent directory. Perhaps something like

remove_dir_all will fail if remove_dir or remove_file fail on any constituent paths, including the root path. As a result, the directory you are deleting must exist, which means this function is not idempotent.

Consider ignoring the error if validating the removal is not required for your use case.

@pitaj
Copy link
Contributor

pitaj commented Jan 27, 2023

I'd prefer to change the behavior instead, if possible. Currently, it appears like this function can easily fail if multiple threads/processes are deleting things. create_dir_all was modified for a similar reason

@philpax
Copy link
Contributor Author

philpax commented Jan 27, 2023

That would also be fine with me, but what would be the desired behaviour be in the cases of a) another thread/process deleting something that this function is about to delete and b) another thread/process creating something that this function is not aware of in its deletion?

The answers to these seem like they might be difficult/OS-dependent.

@pitaj
Copy link
Contributor

pitaj commented Jan 27, 2023

If the directory at path does not exist, immediately exit Ok.

a) another thread/process deleting something that this function is about to delete

Accept ErrorKind::NotFound as if the deletion succeeded.

b) another thread/process creating something that this function is not aware of in its deletion?

If something is created between the read_dir and remove_dir that causes the latter to fail with DirectoryNotEmpty, immediately return that Err.

@philpax
Copy link
Contributor Author

philpax commented Jan 30, 2023

Seems reasonable to me. I'm not in a position to fix up the implementations, but I'm fine with someone else implementing that.

@jyn514
Copy link
Member

jyn514 commented Apr 26, 2023

rust-lang/libs-team#170 has been closed; if I understand correctly, T-libs-api doesn't want to make that change. So I think this documentation change is an improvement :) Can you change the documentation to what you suggested in #105745 (comment) ? r=me with that done.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2023
@jyn514 jyn514 assigned jyn514 and unassigned joshtriplett Apr 26, 2023
@rust-log-analyzer

This comment has been minimized.

@philpax
Copy link
Contributor Author

philpax commented Apr 27, 2023

@rustbot review

(hope I did this right!)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 27, 2023
@jyn514
Copy link
Member

jyn514 commented Apr 27, 2023

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 27, 2023

📌 Commit d5d2785 has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 27, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
docs(std): clarify remove_dir_all errors

When using `remove_dir_all`, I assumed that the function was idempotent and that I could always call it to remove a directory if it existed. That's not the case and it bit me in production, so I figured I'd submit this to clarify the docs.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#105745 (docs(std): clarify remove_dir_all errors)
 - rust-lang#106456 (Correct `std::prelude` comment)
 - rust-lang#106599 (Change memory ordering in System wrapper example)
 - rust-lang#110838 (More `Typefoldable`/`TypeVisitable` cleanups)
 - rust-lang#110851 (compiletest: emit assembly-output header in error)
 - rust-lang#110853 (compiletest: add bpf-linker assembly support)
 - rust-lang#110878 (Add `known-bug` tests for 4 unsound issues)
 - rust-lang#110886 (`DepGraph` cleanups)
 - rust-lang#110905 (Remove invalid value from scraped-examples.md)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e13b7f7 into rust-lang:master Apr 27, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 27, 2023
@philpax philpax deleted the patch-1 branch April 14, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants