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

Convert UNIX std::fs::remove_dir_all() from recursive to looping #93473

Closed

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Jan 30, 2022

std::fs::remove_dir_all() is recursive and can run out of stack space for deep directory hierarchies. This uses @cuviper's looped implementation with an on-heap stack to avoid running out of stack space before running out of file descriptors. As a drive-by fix it reduces the Macos x86-64 special case code.

For easier review the Macos x86-64 commits are broken up in two. One removes the Macos x86-64 special case completely, the second adds more fine-grained special-case handling back.

Further work to make remove_dir_all() use only a fixed number of open file descriptors is planned, see #93160.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Jan 30, 2022
@hkratz hkratz changed the title Convert std::fs::remove_dir_all() from recursive to looping Convert UNIX std::fs::remove_dir_all() from recursive to looping Jan 30, 2022
@cuviper
Copy link
Member

cuviper commented Jan 31, 2022

+1 for consolidating macos -- always nice to see a net reduction in code!

@hkratz
Copy link
Contributor Author

hkratz commented Feb 28, 2022

Needs to be rebased once #94446 is merged.

@rustbot author

@rustbot rustbot 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 Feb 28, 2022
@bors
Copy link
Contributor

bors commented Mar 5, 2022

☔ The latest upstream changes (presumably #94634) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

@hkratz

Needs to be rebased once #94446 is merged.

Triage: Looks like this is unblocked - can you post your status on this PR?

@hkratz
Copy link
Contributor Author

hkratz commented Apr 11, 2022

Closing in favor of #95925.

@hkratz hkratz closed this Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants