Skip to content

GH-89727: Fix shutil.rmtree() recursion error on deep trees#119808

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.

Sign up for GitHub

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

Jump to bottom
Merged
barneygale merged 13 commits into from
Jun 1, 2024
Merged

GH-89727: Fix shutil.rmtree() recursion error on deep trees #119808

barneygale merged 13 commits into from
Jun 1, 2024

Conversation

@barneygale
Copy link
Contributor

@barneygalebarneygale commented May 30, 2024•
edited by bedevere-app bot
Loading

Implement shutil._rmtree_safe_fd() using a list as a stack to avoid emitting recursion errors on deeply nested trees.

shutil._rmtree_unsafe() was fixed in #119634.

@barneygalebarneygale added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels May 30, 2024
@bedevere-appbedevere-appbot mentioned this pull request May 30, 2024
Open
@barneygale
Copy link
ContributorAuthor

barneygale commented May 30, 2024

I tried to implement this atop os.fwalk(), but I couldn't get it to work. There's not enough context around exceptions raised from os.fwalk() basically.

ambv
ambv reviewed May 31, 2024
View reviewed changes
Copy link
Contributor

@ambvambv left a comment

Choose a reason for hiding this comment

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

I like how this is more concise and protects against recursion errors. It's somewhat less obvious what's going on here now, I left some comments.

ambv
ambv approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@ambvambv left a comment

Choose a reason for hiding this comment

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

LGTM. I'll let Jelle comment and merge.

barneygale
barneygale commented May 31, 2024
View reviewed changes
@barneygale
Copy link
ContributorAuthor

barneygale commented Jun 1, 2024•
edited
Loading

Thanks very much for the reviews, both! @JelleZijlstra can I merge, or would you like to do the honours as Łukasz suggested?

This will hit a conflict when backported to 3.12 due to a difference in FileNotFoundError handling. Should be an easy fix.

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Jun 1, 2024

Feel free to merge yourself!

@barneygale
Copy link
ContributorAuthor

barneygale commented Jun 1, 2024

Super. Thanks again!

@barneygalebarneygale merged commit 53b1981 into python:mainJun 1, 2024
@miss-islington-app
Copy link

miss-islington-appbot commented Jun 1, 2024

Thanks @barneygale for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 1, 2024
@barneygale@miss-islington
pythonGH-89727: Fixshutil.rmtree()recursion error on deep trees (p…
136d027
…ythonGH-119808) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679. (cherry picked from commit 53b1981) Co-authored-by: Barney Gale <barney.gale@gmail.com>
@bedevere-app
Copy link

bedevere-appbot commented Jun 1, 2024

GH-119918 is a backport of this pull request to the 3.13 branch.

@miss-islington-app
Copy link

miss-islington-appbot commented Jun 1, 2024

Sorry, @barneygale, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 53b1981fb0cda6c656069e992f172fc6aad7c99c 3.12

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Jun 1, 2024
barneygale added a commit to barneygale/cpython that referenced this pull request Jun 1, 2024
@barneygale
pythonGH-89727: Fixshutil.rmtree()recursion error on deep trees (p…
b900fdb
…ython#119808) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679. (cherry picked from commit 53b1981)
barneygale added a commit to barneygale/cpython that referenced this pull request Jun 1, 2024
@barneygale
[3.12]pythonGH-89727: Fixshutil.rmtree()recursion error on deep …
411a75d
…trees (pythonGH-119808) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679.. (cherry picked from commit 53b1981) Co-authored-by: Barney Gale <barney.gale@gmail.com>
@bedevere-app
Copy link

bedevere-appbot commented Jun 1, 2024

GH-119919 is a backport of this pull request to the 3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12 only security fixes label Jun 1, 2024
barneygale added a commit that referenced this pull request Jun 1, 2024
@miss-islington@barneygale
[3.13]GH-89727: Fixshutil.rmtree()recursion error on deep trees (G…
225c3cd
…H-119808) (#119918) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679. (cherry picked from commit 53b1981) Co-authored-by: Barney Gale <barney.gale@gmail.com>
barneygale added a commit that referenced this pull request Jun 1, 2024
@barneygale
[3.12]GH-89727: Fixshutil.rmtree()recursion error on deep trees (G…
feb7870
…H-119808) (#119919) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679. (cherry picked from commit 53b1981)
barneygale added a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
@barneygale
pythonGH-89727: Fixshutil.rmtree()recursion error on deep trees (p…
39998dd
…ython#119808) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@barneygale@noahbkim
pythonGH-89727: Fixshutil.rmtree()recursion error on deep trees (p…
e2e608a
…ython#119808) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@barneygale@estyxx
pythonGH-89727: Fixshutil.rmtree()recursion error on deep trees (p…
24a1a10
…ython#119808) Implement `shutil._rmtree_safe_fd()` using a list as a stack to avoid emitting recursion errors on deeply nested trees. `shutil._rmtree_unsafe()` was fixed in a150679.
@buhtzbuhtz mentioned this pull request Oct 24, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ambvambvambv approved these changes

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

Assignees

@barneygalebarneygale

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@barneygale@JelleZijlstra@ambv