classification
Title: TemporaryDirectory attempts to clean up twice
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: haypo, josh.r, jwilk, oconnor663, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-09-17 05:18 by oconnor663, last changed 2014-09-24 14:43 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
tempfile_exit_on_shutdown.patch serhiy.storchaka, 2014-09-17 17:34 review
tempfile_exit_on_shutdown2.patch serhiy.storchaka, 2014-09-17 17:56 review
tempfile_exit_on_shutdown3.patch serhiy.storchaka, 2014-09-19 08:51
Messages (12)
msg226979 - (view) Author: Jack O'Connor (oconnor663) * Date: 2014-09-17 05:18
The following little script prints (but ignores) a FileNotFoundError.

import tempfile
def generator():
    with tempfile.TemporaryDirectory():
        yield
g = generator()
next(g)

Exception ignored in: <generator object generator at 0x7fb319fe2c60>
Traceback (most recent call last):
  File "gen.py", line 6, in generator
  File "/usr/lib/python3.4/tempfile.py", line 691, in __exit__
  File "/usr/lib/python3.4/tempfile.py", line 697, in cleanup
  File "/usr/lib/python3.4/shutil.py", line 454, in rmtree
  File "/usr/lib/python3.4/shutil.py", line 452, in rmtree
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp7wek4xhy'

Putting print statements in the TemporaryDirectory class shows what's happening (confirming Guido's theory from https://groups.google.com/forum/#!topic/python-tulip/QXgWH32P2uM): As the program exits, the TemporaryDirectory object is finalized. This actually rmtree's the directory. After that, the generator is finalized, which raises a GeneratorExit inside of it. That exception causes the with statement to call __exit__ on the already-finalized TemporaryDirectory, which tries to rmtree again and throws the FileNotFoundError.

The main downside of this bug is just garbage on stderr. I suppose in exceptionally unlikely circumstances, a new temp dir by the same name could be created between the two calls, and we might actually delete something we shouldn't. The simple fix would be to store a _was_cleaned flag or something on the object. Is there any black magic in finalizers or multithreading that demands something more complicated?
msg226996 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-09-17 12:12
On Python 3.5 compiled in debug mode (or probably with python -Wd when compiled in release mode), I got this warning:

/home/haypo/prog/python/default/Lib/tempfile.py:708: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpcr8u3m8v'>
  _warnings.warn(warn_message, ResourceWarning)

The script is really a corner case: the generator is garbage collected whereas it is not done. At the same time, the TemporaryDirectory object is also garbage collected. I guess that the exact order of object deletion is not reliable: the generator may be deleted before or after the TemporaryDirectory.

TemporaryDirectory._cleanup() is called by the finalizer (_weakref.finalize) of the TemporaryDirectory, which means that the TemporaryDirectory object is garbage collected.

TemporaryDirectory.cleanup() is called indirectly by 
TemporaryDirectory.__exit__().

I'm suprised that deleting a generator calls __exit__().

The following script has a more reliable behaviour: TemporaryDirectory.__exit__() is called when the generator is deleted, and TemporaryDirectory.cleanup() is not called.
---
import tempfile
import gc

def generator():
    with tempfile.TemporaryDirectory():
        print("before yield")
        yield
        print("after yield")
g = generator()
next(g)

g = None
gc.collect()
---
msg227006 - (view) Author: Jack O'Connor (oconnor663) * Date: 2014-09-17 15:10
My example script is definitely a corner case, but where this actually came up for me was in asyncio. Using a TemporaryDirectory inside a coroutine creates a similar situation.
msg227012 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-17 17:34
Here is a patch which may fix this issue.
msg227013 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-17 17:56
Here is alternative patch. It simplifies TemporaryDirectory instead of complicate it.
msg227085 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-19 08:51
Fixed a bug in the test and partially addressed Victor's and Yury's comments.

Antoine, could your pleas answer following question?

Is it safe to remove the "self._finalizer is not None" check in cleanup()? I.e. is it possible that self._finalizer becomes None at garbage collection?
msg227088 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-09-19 10:52
self._finalizer can be None if an exception was raised during __init__().
msg227089 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-19 11:22
But in this case cleanup() can't be called.
msg227099 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-09-19 14:13
Ah... you are right. It seems the None test has been superfluous all the time.
msg227435 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-24 10:31
New changeset 7ea2153eae87 by Serhiy Storchaka in branch '3.4':
Issue #22427: TemporaryDirectory no longer attempts to clean up twice when
https://hg.python.org/cpython/rev/7ea2153eae87

New changeset e9d4288c32de by Serhiy Storchaka in branch 'default':
Issue #22427: TemporaryDirectory no longer attempts to clean up twice when
https://hg.python.org/cpython/rev/e9d4288c32de
msg227438 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-24 10:37
Committed without this test.

Thank you Victor and Yury for your comments.
msg227456 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-09-24 14:43
Cool, the final code is simpler than before!
History
Date User Action Args
2014-09-24 14:43:42hayposetmessages: + msg227456
2014-09-24 10:37:25serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg227438

stage: patch review -> resolved
2014-09-24 10:31:21python-devsetnosy: + python-dev
messages: + msg227435
2014-09-23 21:01:02serhiy.storchakasetassignee: serhiy.storchaka
2014-09-19 14:13:24pitrousetmessages: + msg227099
2014-09-19 11:22:39serhiy.storchakasetmessages: + msg227089
2014-09-19 10:52:21pitrousetmessages: + msg227088
2014-09-19 08:51:28serhiy.storchakasetfiles: + tempfile_exit_on_shutdown3.patch

messages: + msg227085
2014-09-18 18:06:44jwilksetnosy: + jwilk
2014-09-17 17:56:17serhiy.storchakasetfiles: + tempfile_exit_on_shutdown2.patch

messages: + msg227013
2014-09-17 17:34:27serhiy.storchakasetfiles: + tempfile_exit_on_shutdown.patch

versions: + Python 3.5
keywords: + patch
nosy: + serhiy.storchaka, pitrou

messages: + msg227012
stage: patch review
2014-09-17 15:10:19oconnor663setmessages: + msg227006
2014-09-17 12:12:25hayposetmessages: + msg226996
2014-09-17 09:44:08hayposetnosy: + haypo
2014-09-17 08:57:46josh.rsetnosy: + josh.r
2014-09-17 05:18:39oconnor663create