classification
Title: tempfile.TemporaryDirectory fails to delete itself
Type: behavior Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: CAM-Gerlach, Jeffrey.Kintscher, gvanrossum, max, paul.moore, serhiy.storchaka, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2017-04-04 18:26 by max, last changed 2021-03-14 18:11 by gvanrossum. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 24793 merged CAM-Gerlach, 2021-03-08 23:04
Messages (6)
msg291130 - (view) Author: Max (max) * Date: 2017-04-04 18:26
There's a known issue with `shutil.rmtree` on Windows, in that it fails intermittently. 

The issue is well known (https://mail.python.org/pipermail/python-dev/2013-September/128353.html), and the agreement is that it cannot be cleanly solved inside `shutil` and should instead be solved by the calling app. Specifically, python devs themselves faced it in their test suite and solved it by retrying delete.

However, what to do about `tempfile.TemporaryDirectory`? Is it considered the calling app, and therefore should retry delete when it calls `shutil.rmtree` in its `cleanup` method?

I don't think `tempfile` is protected by the same argument that `shutil.rmtree` is protected, in that it's too messy to solve it in the standard library. My rationale is that while it's very easy for the end user to retry `shutil.rmtree`, it's far more difficult to fix the problem with `tempfile.TempDirectory` not deleting itself - how would the end user retry the `cleanup` method (which is called from `weakref.finalizer`)?

So perhaps the retry loop should be added to `cleanup`.
msg292562 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-04-29 04:17
A simpler approach would be to simply ignore the error when it occurs in TemporaryDirectory._cleanup. There are no absolute guarantees about tempfile always cleaning up -- just a best effort. The complexity (and potential delays) of a retry loop seem more risky than simply occasionally not cleaning up -- that happens anyways, for a variety of reasons.
msg388219 - (view) Author: C.A.M. Gerlach (CAM-Gerlach) * Date: 2021-03-07 03:09
In addition to transient failures, this can also occur when, for example, files opened in the temporary directory (perhaps by library or application code not under direct control of the caller) haven't been properly cleaned up and their file handles don't get closed, resulting in permissions errors trying to delete them (particularly on platforms like Windows, that automatically lock files when opening them).

This case came up in e.g. [this recent PR](https://github.com/regebro/pyroma/pull/57) and rendered `tempfile.TemporaryDirectory` unusable for such use cases, forcing a reversion to the lower-level `tempfile.mkdtemp` without the cleaner, more robust and easier to interpret high-level interface that the former provides. Wrapping a `with` statement in a try-finally is syntactically ugly and semantically incongruous, and requires a second shutil.rmtree(..., ignore_errors=True)` call to clean up in a best-effort manner, while when manually calling `cleanup()` in a try-except, the finalizer still gets executed at a a non-deterministic later time when Python exits, raising an error.

Therefore, in the spirit of Guido's statements above in terms of providing a "best-effort" cleanup, I propose (and am willing to submit a PR to implement) adding an `ignore_errors` bool parameter (defaulting to False, of course, for backward compat--and should it be keyword only like `errors` to `TemporaryFile`?) to the `tempfile.TemporaryDirectory` constructor, which gets  passed to `shutil.rmtree` on cleanup. This would not only address both cases here, but also one of the two discussed by Anthony Sotitle on [BPO-25024](https://bugs.python.org/issue25024), in a cleaner and simpler fashion that would take advantage of existing `tempfile.TemporaryDirectory` functionality and behavior.

Would a PR be accepted on this? If so, any specific guidance on tests and whether to mention it in What's New, etc., would be appreciated. Thanks!
msg388246 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-03-07 18:05
SGTM
msg388318 - (view) Author: C.A.M. Gerlach (CAM-Gerlach) * Date: 2021-03-08 23:06
Thanks! Submitted as PR #24793 https://github.com/python/cpython/pull/24793
msg388680 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-03-14 18:07
New changeset bd2fa3c416ffe6107b500a2180fa1764645c0a61 by CAM Gerlach in branch 'master':
bpo-29982: Add "ignore_cleanup_errors" param to tempfile.TemporaryDirectory() (GH-24793)
https://github.com/python/cpython/commit/bd2fa3c416ffe6107b500a2180fa1764645c0a61
History
Date User Action Args
2021-03-14 18:11:24gvanrossumsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-03-14 18:07:04gvanrossumsetmessages: + msg388680
2021-03-08 23:06:16CAM-Gerlachsetmessages: + msg388318
2021-03-08 23:04:18CAM-Gerlachsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23560
2021-03-07 18:05:34gvanrossumsetmessages: + msg388246
2021-03-07 03:09:48CAM-Gerlachsetnosy: + CAM-Gerlach
messages: + msg388219
2019-06-12 23:23:48Jeffrey.Kintschersetnosy: + Jeffrey.Kintscher
2018-11-03 20:05:56serhiy.storchakasetassignee: serhiy.storchaka

nosy: + serhiy.storchaka
2017-04-29 04:17:34gvanrossumsetnosy: + gvanrossum
messages: + msg292562
2017-04-04 20:24:46eryksunsetnosy: + paul.moore, tim.golden, zach.ware, steve.dower

components: + Windows
versions: + Python 3.7
2017-04-04 18:26:18maxcreate