Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tempfile.TemporaryDirectory fails to delete itself #74168

Closed
pkch mannequin opened this issue Apr 4, 2017 · 6 comments
Closed

tempfile.TemporaryDirectory fails to delete itself #74168

pkch mannequin opened this issue Apr 4, 2017 · 6 comments
Assignees
Labels
3.7 (EOL) end of life OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@pkch
Copy link
Mannequin

pkch mannequin commented Apr 4, 2017

BPO 29982
Nosy @gvanrossum, @pfmoore, @tjguk, @pkch, @zware, @serhiy-storchaka, @zooba, @CAM-Gerlach, @websurfer5
PRs
  • bpo-29982: Add "ignore_cleanup_errors" param to tempfile.TemporaryDirectory() #24793
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2021-03-14.18:11:24.171>
    created_at = <Date 2017-04-04.18:26:18.456>
    labels = ['3.7', 'type-bug', 'library', 'OS-windows']
    title = 'tempfile.TemporaryDirectory fails to delete itself'
    updated_at = <Date 2021-03-14.18:11:24.171>
    user = 'https://github.com/pkch'

    bugs.python.org fields:

    activity = <Date 2021-03-14.18:11:24.171>
    actor = 'gvanrossum'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2021-03-14.18:11:24.171>
    closer = 'gvanrossum'
    components = ['Library (Lib)', 'Windows']
    creation = <Date 2017-04-04.18:26:18.456>
    creator = 'max'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29982
    keywords = ['patch']
    message_count = 6.0
    messages = ['291130', '292562', '388219', '388246', '388318', '388680']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'paul.moore', 'tim.golden', 'max', 'zach.ware', 'serhiy.storchaka', 'steve.dower', 'CAM-Gerlach', 'Jeffrey.Kintscher']
    pr_nums = ['24793']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29982'
    versions = ['Python 3.6', 'Python 3.7']

    @pkch
    Copy link
    Mannequin Author

    pkch mannequin commented Apr 4, 2017

    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.

    @pkch pkch mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 4, 2017
    @gvanrossum
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka self-assigned this Nov 3, 2018
    @CAM-Gerlach
    Copy link
    Member

    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 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 callingcleanup()` 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, 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!

    @gvanrossum
    Copy link
    Member

    SGTM

    @CAM-Gerlach
    Copy link
    Member

    Thanks! Submitted as PR bpo-24793 #24793

    @gvanrossum
    Copy link
    Member

    New changeset bd2fa3c by CAM Gerlach in branch 'master':
    bpo-29982: Add "ignore_cleanup_errors" param to tempfile.TemporaryDirectory() (GH-24793)
    bd2fa3c

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    mthrok added a commit to mthrok/audio that referenced this issue May 11, 2022
    On CircleCI, Windows unittests are failing for Python 3.7 with
    `PermissionError` at the end of test when it cleans up temporary
    directory.
    
    According to the discussion python/cpython#74168,
    this is caused by a known issue with `shutil.rmtree`.
    
    In the above thread it is advised to simply ignore the error as it
    is not guaranteed that temp directories are cleaned up.
    
    This commit follows the same path and simply ignore the error
    so that our CI gets back to green.
    mthrok added a commit to mthrok/audio that referenced this issue May 11, 2022
    On CircleCI, Windows unittests are failing for Python 3.7 with
    `PermissionError` at the end of test when it cleans up temporary
    directory.
    
    According to the discussion python/cpython#74168,
    this is caused by a known issue with `shutil.rmtree`.
    
    In the above thread it is advised to simply ignore the error as it
    is not guaranteed that temp directories are cleaned up.
    
    This commit follows the same path and simply ignore the error
    so that our CI gets back to green.
    facebook-github-bot pushed a commit to pytorch/audio that referenced this issue May 11, 2022
    Summary:
    On CircleCI, Windows unittests are failing for Python 3.7 with
    `PermissionError` at the end of test when it cleans up temporary
    directory.
    
    According to the discussion python/cpython#74168,
    this is caused by a known issue with `shutil.rmtree`.
    
    In the above thread it is advised to simply ignore the error as it
    is not guaranteed that temp directories are cleaned up.
    
    This commit follows the same path and simply ignore the error
    so that our CI gets back to green.
    
    Pull Request resolved: #2379
    
    Reviewed By: carolineechen
    
    Differential Revision: D36305595
    
    Pulled By: mthrok
    
    fbshipit-source-id: d9049c2ee3447712119786311f639a1f9f8911c5
    fumitoh added a commit to fumitoh/modelx that referenced this issue Feb 18, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants