Issue35951
Created on 2019-02-09 17:10 by chris.jerdonek, last changed 2022-04-11 14:59 by admin.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 11827 | closed | nanjekyejoannah, 2019-02-12 15:11 | |
| Messages (10) | |||
|---|---|---|---|
| msg335134 - (view) | Author: Chris Jerdonek (chris.jerdonek) * ![]() |
Date: 2019-02-09 17:10 | |
os.renames() creates and leaves behind the intermediate directories if the original (source) path doesn't exist.
>>> import os
>>> os.mkdir('temp')
>>> os.mkdir('temp/test')
>>> os.renames('temp/not-exists', 'temp/test2/test3/test4')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/.../3.6.5/lib/python3.6/os.py", line 267, in renames
rename(old, new)
FileNotFoundError: [Errno 2] No such file or directory: 'temp/not-exists' -> 'temp/test2/test3/test4'
>>> os.listdir('temp/test2')
['test3']
>>> os.listdir('temp/test2/test3')
[]
|
|||
| msg335186 - (view) | Author: Ammar Askar (ammar2) * ![]() |
Date: 2019-02-11 01:33 | |
It seems this behavior is somewhat documented: https://docs.python.org/3/library/os.html#os.renames >Works like rename(), except creation of any intermediate directories needed to make the new pathname good is attempted first. >This function can fail with the new directory structure made if you lack permissions needed to remove the leaf directory or file. The source directory not existing isn't the same as not having permissions to remove it but close enough. |
|||
| msg335192 - (view) | Author: Chris Jerdonek (chris.jerdonek) * ![]() |
Date: 2019-02-11 03:07 | |
Lacking permissions seems very different to me from the source directory or file not existing. For example, in the example I provided, I did have the needed permissions. Incidentally (and this is a separate documentation issue), the note seems unclear as to whether "the leaf directory or file" the user lacks permissions to remove is in reference to the "rightmost path segments of the old name" being pruned away, or the new directory structure to be removed on failure. |
|||
| msg335193 - (view) | Author: Ammar Askar (ammar2) * ![]() |
Date: 2019-02-11 03:18 | |
Aah, I interpreted the combination of the warning and the fact that it says "attempted first" to mean that any failures in the actual renaming will leave the new directory in place. That is, no cleanup is ever performed. A quick glance at the code seems to suggest that as well. |
|||
| msg335374 - (view) | Author: Giampaolo Rodola' (giampaolo.rodola) * ![]() |
Date: 2019-02-12 22:32 | |
The proposed change makes the problem a lot less likely to occur, but technically it doesn't fix it because if the src file/dir disappears between "os.path.exists(src)" and os.rename(src, dst)" calls you'll end up with a race condition. We may still want to do it, but can't make promises about full reliability. Also, it seems to me this behavior should be expected, because the doc explains the whole thing basically happens as a 3-step process (create dst dirs, rename, remove old src path). As such the cleanup in case of failure should not be expected, nor is promised. Also, FileNotFoundError is just one. os.rename() may fail for other reasons (and still leave the dst directory tree behind). If there is something we can do here is probably make the doc more clear (it talks about "lack of permissions" when instead it should have talked on "any error". Extra (unrelated): as I commented on the PR, there are no unit-tests for os.renames(). |
|||
| msg335377 - (view) | Author: Chris Jerdonek (chris.jerdonek) * ![]() |
Date: 2019-02-12 23:57 | |
> As such the cleanup in case of failure should not be expected,
Given that the documentation specifically calls out permissions errors as a cause of leaving the new directory in place, it wouldn't be unreasonable for someone to think the function does a cleanup step on failure using the same pruning process on the target directory.
In fact, that scenario is the one scenario I can think of that makes how the note is written make sense -- the cleanup step not having permissions to remove the intermediate directories it just created ("This function can fail with the new directory structure made if you lack permissions needed to remove the leaf directory or file [just created]").
|
|||
| msg335398 - (view) | Author: Joannah Nanjekye (nanjekyejoannah) * ![]() |
Date: 2019-02-13 09:54 | |
@giampaolo.rodola thanks for insight on tests. I dint catch that at all :) I am working on tests for os.renames() I opened an issue to track that here : https://bugs.python.org/issue35982. IMHO, I think we need to improve the current behavior. This discussion is not yet conclusive. You pointed out that this fix makes the problem a lot less likely to occur but we may end up with a race condition. Can we close this PR so that maybe someone comes with a fix that addresses both problems? can we work with this fix? Am happy to comply with any decision. cc @vstinner Once we agree on this, I will make necessary changes also in the review of this PR. |
|||
| msg335758 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2019-02-17 07:18 | |
I agree with Giampaolo. The proposed change does not solve the problem completely. And since it seems that it can not be solved completely, I suggest to just better document the current behavior. |
|||
| msg335761 - (view) | Author: Chris Jerdonek (chris.jerdonek) * ![]() |
Date: 2019-02-17 08:10 | |
> And since it seems that it can not be solved completely, You may be right only to document, but you didn't note any problems with the possibility I suggested. A cleanup pruning step could be done on failure that is similar to the cleanup pruning step on success. |
|||
| msg335769 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2019-02-17 11:12 | |
There are possible race conditions. Other process can create the same target directory (if it does not exist yet) by calling os.makedirs() for example. It will be impolite to remove the directory just after the second process checked that it exists (or even after it created it). Also, the created directory will left if the program crash before deleting it. os.renames() to non-existing directory can not be atomic. It can interfere with other processes or threads. We should just document this. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:11 | admin | set | github: 80132 |
| 2019-02-17 11:12:33 | serhiy.storchaka | set | messages: + msg335769 |
| 2019-02-17 08:10:41 | chris.jerdonek | set | messages: + msg335761 |
| 2019-02-17 07:18:34 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages: + msg335758 |
| 2019-02-13 09:54:57 | nanjekyejoannah | set | nosy:
+ nanjekyejoannah messages: + msg335398 |
| 2019-02-12 23:57:07 | chris.jerdonek | set | messages: + msg335377 |
| 2019-02-12 22:32:26 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola messages: + msg335374 |
| 2019-02-12 15:20:21 | vstinner | set | versions: + Python 3.8, - Python 3.6 |
| 2019-02-12 15:11:38 | nanjekyejoannah | set | keywords:
+ patch stage: patch review pull_requests: + pull_request11858 |
| 2019-02-11 03:18:52 | ammar2 | set | messages: + msg335193 |
| 2019-02-11 03:07:58 | chris.jerdonek | set | messages: + msg335192 |
| 2019-02-11 01:33:52 | ammar2 | set | nosy:
+ ammar2 messages: + msg335186 |
| 2019-02-09 17:10:42 | chris.jerdonek | create | |
