bpo-43153: Don't mask PermissionError with NotADirectoryError during tempdirectory cleanup - #29940

I think this would also happen on other OSes, not just Windows. The bug can only be reproduced on windows because PermissionError in the bug was caused by an open file, but PermissionError in theory can be caused by actual permission / ownership settings. It shouldn't matter much because this is a temp dir (and files in it) created by current user, but this comment can be misunderstood, so it's better to drop ref. to Windows here.

It may be left in the if case below because it can avoid the isdir() check.

It would be good to also add "Issue #43153" to the comment.

Note that calling unlink() on a directory in Windows always raises PermissionError. Windows maps the NT status code STATUS_FILE_IS_A_DIRECTORY to the WinAPI error code ERROR_ACCESS_DENIED.

I think this would also happen on other OSes, not just Windows. The bug can only be reproduced on windows

I'm confused, should we continue the _os.name == 'nt' check then? I'm not worried about the additional performance hit from isdir (though I would like to avoid it). Correctness is more important. If this does happen on other OSes, then we should remove that OS check.

We can remove the os check. It can happen on other OSes. With the check removed, the logic becomes "if we are not permitted to remove the file, return or reraise", which makes perfect sense.

os.path.isdir() returns True for a symlink to directory. Either check os.path.islink() before os.path.isdir() or use os.stat() explicitly.

Failing to delete a symlink should be rare in a temp directory. But if it's worth handling here, then it should be consistent with shutil._rmtree_isdir(). Unfortunately the latter is written for os.DirEntry instances. Here's an updated implementation for shutil that can also check a path. I factored out the check for a mount point into a common _st_ismount(st) function, which avoids duplicated code and, I think, makes it easier to read.

if not hasattr(os.stat_result, 'st_reparse_tag'):
    def _rmtree_islink(path):
        return os.path.islink(path)

    def _rmtree_isdir(path):
        try:
            if isinstance(path, os.DirEntry):
                return entry.is_dir(follow_symlinks=False)
            return stat.S_ISDIR(os.lstat(path).st_mode)
        except OSError:
            return False
else:
    # In general mount points in Windows are directories, but os.unlink()
    # supports them, so rmtree() handles them as symlinks.
    def _st_ismount(st):
        return (st.st_file_attributes & stat.FILE_ATTRIBUTE_REPARSE_POINT
                and st.st_reparse_tag == stat.IO_REPARSE_TAG_MOUNT_POINT)

    def _rmtree_islink(path):
        try:
            if isinstance(path, os.DirEntry):
                st = entry.stat(follow_symlinks=False)
            else:
                st = os.lstat(path)
            return stat.S_ISLNK(st.st_mode) or _st_ismount(st)
        except OSError:
            return False

    def _rmtree_isdir(path):
        try:
            if isinstance(path, os.DirEntry):
                st = entry.stat(follow_symlinks=False)
            else:
                st = os.lstat(path)
            return stat.S_ISDIR(st.st_mode) and not _st_ismount(st)
        except OSError:
            return False

Why not go with an "ask-for-forgiveness-rather-than-permission" type of solution, e.g.:

try:
    _os.unlink(path)
except IsADirectoryError:
    cls._rmtree(path, ignore_errors=ignore_errors)
except PermissionError as pe:
    # PermissionError is raised on FreeBSD for directories
    # and by Windows on lock files used by other processes
    try:
        cls._rmtree(path, ignore_errors=ignore_errors)
    except NotADirectoryError:
        # NOTE: This is raised if PermissionError did not
        # correspond to a IsADirectoryError, e.g. on
        # Windows.
        if not ignore_errors:
            raise pe

rmtree() is a complex function, and we cannot be sure whether NotADirectoryError was raised by the toplevel scandir() or somewhere deeper in the tree.

Mmmh...

Seems like a responsibility inversion to me. Sure there could be a bug deep down in _rmtree that ends up bubbling an unrelated NotADirectoryError, but that would technically be a bug in _rmtree. Maybe there are valid cases where such an unrelated error would be raised?

Also I see the diff I commented on is not the final version so I'll look at the final version.

Not necessary a bug. It may be a race condition. Although the LBYL approach is also prone to race conditions, I think it has less chance to override error with a wrong exception.

I myself prefer the EAFP approach, but I do not think that it has advantages in this case.