gh-120882: Fix shutil.move to properly handle files opened by another process#120883
gh-120882: Fix shutil.move to properly handle files opened by another process#120883Saqibs575 wants to merge 1 commit intopython:mainfrom Saqibs575:fix/shutil-OSError-handling
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Unfortunately we can't know that ahead of time, nor can we roll back the copy if we encounter an error while deleting. |
| except OSError: | ||
| except OSError as exc: | ||
| if exc.errno == errno.EACCES: | ||
| raise |
There was a problem hiding this comment.
| raise | |
| raise |
|
It doesn't raise the correct error when moving a directory on Windows (you should check that first): ======================================================================
ERROR: test_dont_move_dir_in_itself (test.test_shutil.TestMove.test_dont_move_dir_in_itself)
----------------------------------------------------------------------
Traceback (most recent call last):
File "D:\a\cpython\cpython\Lib\test\test_shutil.py", line 2742, in test_dont_move_dir_in_itself
self.assertRaises(shutil.Error, shutil.move, self.src_dir, dst)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\a\cpython\cpython\Lib\unittest\case.py", line 804, in assertRaises
return context.handle('assertRaises', args, kwargs)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\a\cpython\cpython\Lib\unittest\case.py", line 238, in handle
callable_obj(*args, **kwargs)
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
File "D:\a\cpython\cpython\Lib\shutil.py", line 857, in move
os.rename(src, real_dst)
~~~~~~~~~^^^^^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\a\\cpython\\cpython\\build\\test_python_[833](https://github.com/python/cpython/actions/runs/9626446182/job/26552425077?pr=120883#step:6:834)6�\\tmpjagpnbs_' -> 'D:\\a\\cpython\\cpython\\build\\test_python_8336�\\tmpjagpnbs_\\bar'Could you also add a test for the fix? |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
This change is wrong. You cannot simply ignore all OSErrors but EACCESS.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Note that starting with Windows 11 22H2, this test case fails with What's changed recently is that for a rename operation, the kernel's internal I/O manager function With the old implementation, which dates back about 30 years, the internal Note that trying to rename a directory into a subdirectory of itself (e.g. "spam" -> "spam\eggs\foo") still fails with Footnotes
|
5 participants
Bug report
Bug description:
Problem:
When attempting to move a file using shutil.move from one location to another, if the file is opened by another process (if file is opened in other python script using
openfunction), shutil.move raises an error (WinError 32: The process cannot access the file because it is being used by another process). Despite this error, the file is still copied to the destination directory, which is unexpected behavior.Details:
The shutil.move function is designed to handle moving files or directories between locations. However, it does not check if the file is currently open by another process before attempting to move it. This results in an error being raised, but the file is still copied to destination regardless of the error condition.
Solution:
To address this issue, I modified the shutil.move function to include an additional check for OSError with errno.EACCES (Permission Denied) in the exception handling block. This ensures that if the file cannot be moved due to being opened by another process, the operation is aborted cleanly without copying the file to maintain the integrity of the move function as well as file system.
Here is the modified part of the shutil.move function
Proposal:
I propose adding this error handling improvement to the shutil.move function in Python's standard library. This enhancement will make file operations more reliable in scenarios where files may be concurrently accessed by multiple processes
Steps to Reproduce:
openfunction in read mode.NOTE: Avoid to use context manager here for opening file to find the error.
You can also run the following script to get the error.
Expected Behavior:
When shutil.move encounters a situation where the source file is open in another process (WinError 32), it should raise an exception and abort the operation without performing any partial file copying to the destination.
Impact:
This issue affects users trying to use shutil.move to relocate files that are concurrently accessed by other processes. The proposed solution aims to improve the robustness and expected behavior of the function in such scenarios.
CPython versions tested on:
3.10
Operating systems tested on:
Windows