GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-89727: Fix `shutil.rmtree()` recursion error on deep trees by barneygale · Pull Request #119808 · python/cpython

Conversation

@barneygale

@barneygale barneygale commented May 30, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Change shutil.rmtree and os.walk to support very deep hierarchies #89727

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
barneygale added 4 commits May 30, 2024 07:53
@barneygale
Tidy up _rmtree_safe_fd() implementation.
417b829
@barneygale
Rewrite using list as stack.
8d42ea8
@barneygale
Add NEWS
7018370
@barneygale
Enable test
9f13194
@barneygale barneygale added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels May 30, 2024
@barneygale
barneygale requested a review from JelleZijlstra May 30, 2024 20:42
@bedevere-app bedevere-app Bot added the awaiting core review label May 30, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 30, 2024
Change shutil.rmtree and os.walk to support very deep hierarchies #89727
Open
@barneygale

barneygale commented May 30, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale added 4 commits May 30, 2024 21:56
@barneygale
Use DirEntry.stat() where possible.
aee87db
@barneygale
Comments, naming, line limit.
d9d0386
@barneygale
Restore comment.
4daf2f2
@barneygale
Simplify overall diff
a335a63
ambv
ambv reviewed May 31, 2024
View reviewed changes

@ambv ambv left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread Lib/shutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/shutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/shutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/shutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
barneygale added 4 commits May 31, 2024 12:54
@barneygale
Address some review feedback
1d4772a
@barneygale
Handle further errors in finally: block
462cfc1
@barneygale
Comments.
8d7ba83
@barneygale
Handle os.close explicitly
11deeab
ambv
ambv approved these changes May 31, 2024
View reviewed changes

@ambv ambv left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels May 31, 2024
@barneygale
Skip os.unlink() if os.DirEntry.is_dir() raises FileNotFoundError
a53e473
barneygale
barneygale commented May 31, 2024
View reviewed changes
Comment thread Lib/shutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra approved these changes Jun 1, 2024
View reviewed changes
@barneygale

barneygale commented Jun 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra

JelleZijlstra commented Jun 1, 2024

Copy link
Copy Markdown
Member

Feel free to merge yourself!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale

barneygale commented Jun 1, 2024

Copy link
Copy Markdown
Contributor Author

Super. Thanks again!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
barneygale merged commit 53b1981 into python:main Jun 1, 2024
@miss-islington-app

miss-islington-app Bot commented Jun 1, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Jun 1, 2024
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

bedevere-app Bot commented Jun 1, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Jun 1, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jun 1, 2024
@miss-islington-app miss-islington-app Bot assigned barneygale 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

bedevere-app Bot commented Jun 1, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot 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: 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.
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
Test failure with "shutil.rmtree()" in isolated build environments (1.5.3.rc1 in Fedora 41) bit-team/backintime#1911
Closed
@qodo-code-review qodo-code-review Bot mentioned this pull request Sep 18, 2026
Eliminate protected payload persistence and gate security evidence ktogias/gnostoa#278
Merged
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@barneygale barneygale

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@barneygale @JelleZijlstra @ambv
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.