GeezerVison

Most changes to Python require a NEWS entry.

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

Most changes to Python require a NEWS entry.

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

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

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:
            for fd in reversed(fd_stack):
                try:
                    close(fd)
                except OSError:
                    pass
            raise

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.

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

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.

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

@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 :)