python / cpython

The Python programming language
https://www.python.org
Other
62.59k stars 30.03k forks source link

tempfile doesn't seem to play nicely with os.chdir on Windows #86962

Open c9f73e94-5cae-4a7c-81f2-f7364bbee5df opened 3 years ago

c9f73e94-5cae-4a7c-81f2-f7364bbee5df commented 3 years ago
BPO 42796
Nosy @tiran, @eryksun, @P403n1x87
Files
  • tempfile_st.txt: Stacktrace
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['library', '3.9', 'type-crash'] title = "tempfile doesn't seem to play nicely with os.chdir on Windows" updated_at = user = 'https://github.com/P403n1x87' ``` bugs.python.org fields: ```python activity = actor = 'eryksun' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'Gabriele Tornetta' dependencies = [] files = ['49712'] hgrepos = [] issue_num = 42796 keywords = [] message_count = 5.0 messages = ['384125', '384165', '384166', '384168', '384172'] nosy_count = 3.0 nosy_names = ['christian.heimes', 'eryksun', 'Gabriele Tornetta'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'crash' url = 'https://bugs.python.org/issue42796' versions = ['Python 3.9'] ```

    c9f73e94-5cae-4a7c-81f2-f7364bbee5df commented 3 years ago

    The following script causes havoc on Windows while it works as expected on Linux

    import os
    import tempfile
    
    ```py
    def test_chdir():
        with tempfile.TemporaryDirectory() as tempdir:
            os.chdir(tempdir)
    
    
    Running the above on Windows results in RecursionError: maximum recursion depth exceeded while calling a Python object (see attachment for the full stacktrace).
    tiran commented 3 years ago

    The code fails because TemporaryDirectory.__exit__() is unable to remove the directory. Windows doesn't let you remove files and directories that are used by a process. On POSIX-like operating systems like Linux support removing of opened files. For example anonymous temporary files use the trick.

    c9f73e94-5cae-4a7c-81f2-f7364bbee5df commented 3 years ago

    That makes sense, but I wonder what the "right" behaviour should be in this case. Surely the infinite recursion should be fixed at the very minimum. Perhaps the code on Windows could be enhanced to catch the case whereby one is trying to delete the cwd and do something like chdir('..') and then delete the temp folder. However, I suspect that something like this still wouldn't be enough. For example, this works fine

    
    def test_chdir():
        with tempfile.TemporaryDirectory() as tempdir:
            old = os.getcwd()
            os.chdir(tempdir)
            os.chdir(old)
    \~\~\~
    
    whereas this doesn't (same stacktrace as the original case)
    
    ~~~ python
    def test_chdir():
        with tempfile.TemporaryDirectory() as tempdir:
            old = os.getcwd()
            os.chdir(tempdir)
            with open(os.path.join(tempdir, "delme")) as fout:
                fout.write("Hello")
            os.chdir(old)
    \~\~\~
    eryksun commented 3 years ago

    Windows doesn't let you remove files and directories that are used by a process.

    Windows does allow deleting open files/directories, but read/execute, write/append, and delete/rename access have to be explicitly shared when opening a file or directory. WinAPI SetCurrentDirectoryW (called by os.chdir) does not share delete access when opening a directory as the new working directory, and the handle is kept open to use as the NTAPI RootDirectory handle when opening relative paths. So the directory can only be deleted after either the working directory changes or the process exits.

    See bpo-35144 for the source of the recursion error. An onerror handler was added that retries rmtree() after attempting to reset permissions. I proposed a workaround in the linked issue, but I don't think it's enough. Maybe there should be a separate onerror function for Windows.

    The two common PermissionError cases to handle in Windows are readonly files and sharing violations. If the readonly attribute is set, we can remove it and retry the unlink() or rmdir() call. I don't think there's no need for a recursive call since rmtree() only removes empty directories.

    For a sharing violation (winerror 32), all that can be done is to set an atexit function that retries deleting the directory. If rmtree() still fails, emit a resource warning and give up.

    eryksun commented 3 years ago

    def test_chdir(): with tempfile.TemporaryDirectory() as tempdir: old = os.getcwd() os.chdir(tempdir) with open(os.path.join(tempdir, "delme")) as fout: fout.write("Hello") os.chdir(old)

    The open() call in test_chdir() fails before os.chdir(old) executes. You would need to call os.chdir(old) in a finally block.

    But the cleanup routine could still fail due to a sharing violation from some other process (likely a child process) setting the directory or a child directory as the working directory. Also, a sharing violation when trying to delete a regular file in the tree causes the cleanup routine to fail with NotADirectoryError because the onerror function calls rmtree() if unlink() fails with a PermissionError.