classification
Title: os.renames() creates directories if original name doesn't exist
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ammar2, chris.jerdonek, giampaolo.rodola, nanjekyejoannah, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2019-02-09 17:10 by chris.jerdonek, last changed 2019-02-17 11:12 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 11827 closed nanjekyejoannah, 2019-02-12 15:11
Messages (10)
msg335134 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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
2019-02-17 11:12:33serhiy.storchakasetmessages: + msg335769
2019-02-17 08:10:41chris.jerdoneksetmessages: + msg335761
2019-02-17 07:18:34serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg335758
2019-02-13 09:54:57nanjekyejoannahsetnosy: + nanjekyejoannah
messages: + msg335398
2019-02-12 23:57:07chris.jerdoneksetmessages: + msg335377
2019-02-12 22:32:26giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg335374
2019-02-12 15:20:21vstinnersetversions: + Python 3.8, - Python 3.6
2019-02-12 15:11:38nanjekyejoannahsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11858
2019-02-11 03:18:52ammar2setmessages: + msg335193
2019-02-11 03:07:58chris.jerdoneksetmessages: + msg335192
2019-02-11 01:33:52ammar2setnosy: + ammar2
messages: + msg335186
2019-02-09 17:10:42chris.jerdonekcreate