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.

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

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

Merged
merged 13 commits into from
Jun 1, 2024
Merged

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

merged 13 commits into from
Jun 1, 2024

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale 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.

@barneygale barneygale added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels May 30, 2024
@bedevere-app bedevere-app bot mentioned this pull request May 30, 2024
Open
@barneygale
Copy link
Contributor Author

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

@ambv ambv 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.

Lib/shutil.py Outdated Show resolved Hide resolved
Lib/shutil.py Outdated Show resolved Hide resolved
Lib/shutil.py Outdated Show resolved Hide resolved
Lib/shutil.py Show resolved Hide resolved
ambv
ambv approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@ambv ambv 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
Copy link
Contributor Author

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

Feel free to merge yourself!

@barneygale
Copy link
Contributor Author

Super. Thanks again!

@barneygale barneygale merged commit 53b1981 into python:main Jun 1, 2024
33 checks passed
@miss-islington-app
Copy link

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: Fix shutil.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 <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jun 1, 2024

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

@miss-islington-app
Copy link

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-app bedevere-app bot 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: Fix shutil.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: Fix shutil.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 <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jun 1, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and 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: Fix shutil.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 <[email protected]>
barneygale added a commit that referenced this pull request Jun 1, 2024
@barneygale
[3.12] GH-89727: Fix shutil.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: Fix shutil.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: Fix shutil.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: Fix shutil.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.
@buhtz buhtz mentioned this pull request Oct 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambv ambv ambv approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Assignees

@barneygale barneygale

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@barneygale @JelleZijlstra @ambv