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

Windows: Iterative remove_dir_all #96412

Merged
merged 3 commits into from
Jun 25, 2022
Merged

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Apr 25, 2022

This will allow better strategies for use of memory and File handles. However, fully taking advantage of that is left to future work.

Note to reviewer: It's probably best to view the remove_dir_all_recursive as a new function. The diff is not very helpful (imho).

This will allow better strategies for use of memory and File handles. However, fully taking advantage of that is left to future work.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 25, 2022
@rust-highfive
Copy link
Collaborator

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 r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. 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

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2022
It's possible that a file in the directory is pending deletion. In that case we might succeed after a few attempts.
@ChrisDenton ChrisDenton added the O-windows Operating system: Windows label Apr 28, 2022
@ChrisDenton
Copy link
Member Author

r? rust-lang/libs

@rust-highfive rust-highfive assigned kennytm and unassigned yaahc Apr 29, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@ChrisDenton
Copy link
Member Author

Two months later, let's maybe reroll this. I'm quite keen to get this merged at some point because it's a perf improvement even without any follow up.

r? rust-lang/libs

@rust-highfive rust-highfive assigned thomcc and unassigned kennytm Jun 24, 2022
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me.

let mut dirlist = vec![f.duplicate()?];

// FIXME: This is a hack so we can push to the dirlist vec after borrowing from it.
fn copy_handle(f: &File) -> mem::ManuallyDrop<File> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gross but I don't see an easy alternative and it's commented with a fixme, so fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I aim to do some bigger changes which I'm hoping will fix this. But my initial attempt was way too big for one PR (it touched a lot of different places and the diff was awful) so this was the compromise solution. I hope it's very temporary.

// Otherwise return the error.
Err(e) => return Err(e),
}
thread::yield_now();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do kind of wonder if it's worth sleeping a bit between these in order to maximize the chance that if it's a temporary failure, it goes away.

I guess in practice this might be good enough, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've had some very mixed feedback about sleeping. Some people seemed quite horrified at the prospect. Though I'm uncertain if it was just the principle of the thing, In any case, I guess it would need some limit to help avoid some possible worst case scenario where we end up sleeping for each and every file. Depending on the version of Windows and the environment, the minimum sleep time may be as much as 16ms.

@thomcc
Copy link
Member

thomcc commented Jun 24, 2022

This makes sense to me, and while there are some nits I don't see great ways to fix them right now, and they don't really matter anyway.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 24, 2022

📌 Commit d579665 has been approved by thomcc

@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 Jun 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 25, 2022
Windows: Iterative `remove_dir_all`

This will allow better strategies for use of memory and File handles. However, fully taking advantage of that is left to future work.

Note to reviewer: It's probably best to view the `remove_dir_all_recursive` as a new function. The diff is not very helpful (imho).
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 25, 2022
Windows: Iterative `remove_dir_all`

This will allow better strategies for use of memory and File handles. However, fully taking advantage of that is left to future work.

Note to reviewer: It's probably best to view the `remove_dir_all_recursive` as a new function. The diff is not very helpful (imho).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#96412 (Windows: Iterative `remove_dir_all`)
 - rust-lang#98126 (Mitigate MMIO stale data vulnerability)
 - rust-lang#98149 (Set relocation_model to Pic on emscripten target)
 - rust-lang#98194 (Leak pthread_{mutex,rwlock}_t if it's dropped while locked.)
 - rust-lang#98298 (Point to type parameter definition when not finding variant, method and associated item)
 - rust-lang#98311 (Reverse folder hierarchy)
 - rust-lang#98401 (Add tracking issues to `--extern` option docs.)
 - rust-lang#98429 (Use correct substs in enum discriminant cast)
 - rust-lang#98431 (Suggest defining variable as mutable on `&mut _` type mismatch in pats)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d7388d1 into rust-lang:master Jun 25, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jun 25, 2022
@ChrisDenton ChrisDenton deleted the remove-dir-all branch June 25, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows 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.

9 participants