gh-89727: Fix os.fwalk RecursionError on deep trees

[jonburdo]

Use a stack to implement os.fwalk iteratively instead of recursively to avoid hitting recursion limits on deeply nested trees.

Similar to how this is done for os.walk in #99803

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

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[jonburdo]

TODO:

• Once gh-89727: Fix os.walk RecursionError on deep trees #99803 is merged, this line can be removed so that FwalkTests will run test_walk_above_recursion_limit to prevent a regression

[jonburdo]

Note that handling of a single exception should function the same, but repeated errors will interrupt the file descriptor cleanup (closing).

Imagine a user holds down ctrl-c sending a burst of KeyboardExceptions. Previously in the recursive version there were nested try-finally statements making sure close(fd) gets called. I believe each exception might cause one of these close(fd) calls to be missed (if it occurs in the finally before close(fd) is executed), but after the burst of exceptions is over, the remaining calls would be made. Now the cleanup is handled in a loop, so a single exception would end the fd cleanup:

try: ... except: forfdinreversed(fd_stack): try: close(fd) exceptOSError: passraise

I'm not sure this is a problem. If I understand correctly, neither versions handles this perfectly. It seemed worth mentioning though.

I thought about changing the except OSError: is this cleanup loop to a bare except:, but then you get kind of a weird, inconsistent behavior where a KeyboardInterrupt that happens to fall just before the try-except starts will end the loop, but one that falls in the try-except won't.

[jonburdo]

Opening this up for review in response to #89727 (comment)

[barneygale]

LGTM, nice work!

[barneygale]

A thought: could you merge _fwalk() into fwalk()? As we're no longer calling _fwalk() recursively, the separation appears pointless, and the yield from indirection will have a small performance cost.

[barneygale]

Could this be a finally: block?

[jonburdo]

Sure, would you consider that more readable or explicit? I just didn't see any benefit, since this is only needed if an exception occurs. The only functional difference would be that we'd iterate over an empty list if no exceptions occur right?

[barneygale]

I think I'd consider it more explicit, though feel free to wait for another opinion if you'd like.

[AlexWaygood]

I'd also prefer finally here. Not sure if it actually delivers any functional benefit here (can't remember the exact semantics off the top of my head...), but it does feel at least more readable to me.

[jonburdo]

Makes sense, thanks for clarifying. I'll change it to finally. Functionally, I think finally is slightly worse but it's negligible.

In this case, changing to finally is functionally equivalent to adding an else clause with the same for-loop:

try: ... except: foraction, valueinreversed(stack): ... raiseelse: foraction, valueinreversed(stack): ...

If no exception occurs in the try, then stack will be empty and all of the fds will have been closed already. So changing to finally means we're adding a step where we simply iterate over an empty list - unnecessary, but costs almost nothing.

To me, a bare except indicates a cleanup step, but finally is even more explicit, so it probably makes sense here.

[barneygale]

@jonburdo I've fixed this in another PR - #119638. Shamefully I forgot about this PR :(.

Comparing our code, I think we arrived at similar solutions, the main difference being that I moved the lstat() of directory entries our of the scandir() loop. If there's anything you think I've missed in my implementation please let me know or log another PR and I'll review. Thanks and sorry.

[jonburdo]

@barneygale No worries! Thanks for mentioning it. I meant to follow up on this. I'll take a look at your PR.

Thanks for working on this function and the related ones! It feels great to see these improvements :)