GH-89727: Fix `os.fwalk()` recursion error on deep trees by barneygale · Pull Request #119638 · python/cpython · GitHub
Skip to content

GH-89727: Fix os.fwalk() recursion error on deep trees#119638

Merged
barneygale merged 9 commits intopython:mainfrom
barneygale:gh-89727-os-fwalk
May 30, 2024
Merged

GH-89727: Fix os.fwalk() recursion error on deep trees#119638
barneygale merged 9 commits intopython:mainfrom
barneygale:gh-89727-os-fwalk

Conversation

@barneygale
Copy link
Copy Markdown
Contributor

@barneygale barneygale commented May 28, 2024

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

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
@barneygale
Copy link
Copy Markdown
Contributor Author

barneygale commented May 28, 2024

Comment thread Lib/os.py Outdated
@barneygale
Copy link
Copy Markdown
Contributor Author

Ready for review! I fixed the test failure mentioned above by reversing the dirs and entries, though IMHO this shouldn't be necessary in bottom-up mode. The diff is fairly large because a lot of work was happening after the recursive call in the previous version, and all that code needed to be brought up front. Fortunately that allows for some de-duplication of open() and stat() calls.

Copy link
Copy Markdown
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

I won't claim I did a line-by-line verification of equivalence with the current code, but I walked through the new code and it looks sensible, and the existing test coverage looks reasonable.

Did you consider adding a test that sets a very low recursion limit and verifies we don't hit it? I don't feel strongly about this.

Comment thread Lib/os.py
@barneygale
Copy link
Copy Markdown
Contributor Author

Did you consider adding a test that sets a very low recursion limit and verifies we don't hit it? I don't feel strongly about this.

I've enabled test_walk_above_recursion_limit for fwalk(), which tests with the recursion limit set to 45 and a directory depth of 50. Previously that test was enabled only for walk(). Unless you mean an even lower recursion limit?

@carljm
Copy link
Copy Markdown
Member

carljm commented May 30, 2024

I've enabled test_walk_above_recursion_limit for fwalk(), which tests with the recursion limit set to 45 and a directory depth of 50. Previously that test was enabled only for walk(). Unless you mean an even lower recursion limit?

No, that looks fine. Sorry, somehow I just totally missed that one-line removal when focused on the "added lines" side of the diff!

@barneygale
Copy link
Copy Markdown
Contributor Author

No worries, thanks tons for the speedy review :)

@barneygale barneygale enabled auto-merge (squash) May 30, 2024 02:22
@barneygale barneygale merged commit 3c890b5 into python:main May 30, 2024
@miss-islington-app
Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 30, 2024
…nGH-119638)

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
(cherry picked from commit 3c890b5)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 30, 2024
…nGH-119638)

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
(cherry picked from commit 3c890b5)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 30, 2024

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label May 30, 2024
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 30, 2024

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label May 30, 2024
barneygale added a commit that referenced this pull request May 30, 2024
…19638) (#119764)

GH-89727: Fix `os.fwalk()` recursion error on deep trees (GH-119638)

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
(cherry picked from commit 3c890b5)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
barneygale added a commit that referenced this pull request May 30, 2024
…19638) (#119765)

GH-89727: Fix `os.fwalk()` recursion error on deep trees (GH-119638)

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
(cherry picked from commit 3c890b5)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
@bedevere-bot
Copy link
Copy Markdown

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…n#119638)

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…n#119638)

Implement `os.fwalk()` using a list as a stack to avoid emitting recursion
errors on deeply nested trees.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants