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 may throw errors at shutdown #54397

Closed
ncoghlan opened this issue Oct 24, 2010 · 14 comments
Closed

tempfile.TemporaryDirectory may throw errors at shutdown #54397

ncoghlan opened this issue Oct 24, 2010 · 14 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@ncoghlan
Copy link
Contributor

BPO 10188
Nosy @birkenfeld, @terryjreedy, @ncoghlan, @pitrou, @merwok, @bitdancer
Files
  • issue10888_tempdir_cleanup.diff: More meaningful error on stderr at shutdown, emit resource warning
  • 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 = None
    closed_at = <Date 2010-12-13.03:13:05.778>
    created_at = <Date 2010-10-24.10:52:58.985>
    labels = ['type-bug']
    title = 'tempfile.TemporaryDirectory may throw errors at shutdown'
    updated_at = <Date 2010-12-13.16:36:47.102>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2010-12-13.16:36:47.102>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-12-13.03:13:05.778>
    closer = 'ncoghlan'
    components = []
    creation = <Date 2010-10-24.10:52:58.985>
    creator = 'ncoghlan'
    dependencies = []
    files = ['20022']
    hgrepos = []
    issue_num = 10188
    keywords = ['patch']
    message_count = 14.0
    messages = ['119505', '119507', '119937', '123807', '123808', '123812', '123821', '123822', '123838', '123839', '123840', '123860', '123880', '123882']
    nosy_count = 7.0
    nosy_names = ['georg.brandl', 'terry.reedy', 'ncoghlan', 'pitrou', 'eric.araujo', 'r.david.murray', 'Jurko.Gospodneti\xc4\x87']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue10188'
    versions = ['Python 3.2']

    @ncoghlan
    Copy link
    Contributor Author

    During interpreter shutdown, modules can become unusable as module globals are set to None. This is a problem for tempfile.TemporaryDirectory, as it needs working os, os.path and stat modules in order to clean up the filesystem.

    The class makes a valiant attempt at reducing the frequency of these errors, but it is ultimately useless, since the three modules internally rely on their various globals remaining valid.

    Issue bpo-812369 may be a possible solution to this problem, or perhaps even an explicit list of essential modules that are nulled out only after all other modules have been destroyed.

    @ncoghlan
    Copy link
    Contributor Author

    Cleanup of sys and __builtin__ is already delayed - this particular issue could be fixed by delaying cleanup of a few more modules, along with the proposed change to GC invocation in issue bpo-1545463.

    @terryjreedy
    Copy link
    Member

    Shutdown has been a 'forever' problem ;-). It would seem sensible to me to have a fixed end-of-shutdown sequence for essential modules.

    @JurkoGospodneti
    Copy link
    Mannequin

    JurkoGospodneti mannequin commented Dec 11, 2010

    Also this class, because it defines __del__ too simply, will display a user-unfriendly error message when cleaning up a TemporaryDirectory object whose constructor raised an exception when attempting to create its temporary folder.

    For example try to create a TemporaryDirectory with prefix="aa>aa" on Windows. That should fail as folders there can not contain '>' characters and later on in the program you should get an error message something like this one:

    Exception AttributeError: "'TemporaryDirectory' object has no attribute '_closed'" in <bound method TemporaryDirectory.cleanup of <tempfile.TemporaryDirectory object at 0x00CE1E10>> ignored

    Hope this helps.

    [Sorry, did not know whether to add this as a separate issue as it seemed kind of related to this one.]

    @JurkoGospodneti
    Copy link
    Mannequin

    JurkoGospodneti mannequin commented Dec 11, 2010

    Clicked send too soon on the previous comment. :-(

    The simplest way I see you can fix the __del__ issue is to patch TemporaryDirectory.__init__() as follows:

        def __init__(self, suffix="", prefix=template, dir=None):
            self._closed = True
            self.name = mkdtemp(suffix, prefix, dir)
            self._closed = False

    This is based on the tempfile.py from the 3.2 beta 1 release on Windows.

    @birkenfeld
    Copy link
    Member

    Applied the fix from msg123808 in r87172.

    @ncoghlan
    Copy link
    Contributor Author

    I have a slightly better fix for that coming soon. There's a subtle race condition in the proposed approach that can lead to the temporary dir not being deleted if an asynchronous exception, such as KeyboardInterrupt, arrives after mkdtemp completes successfully, but before _closed is set back to False.

    Instead, the new init code will look like:

            self._closed = False
            self.name = None # Handle mkdtemp throwing an exception
            self.name = mkdtemp(suffix, prefix, dir)

    And the cleanup condition will be gated on self.name being set as well as on _closed being False. I believe there is still a window where mkdtemp successfully creates the directory, but self.name never gets set, but we want to make that window as small as possible.

    I also realised this should be emitting a ResourceWarning from __del__, as well catching the errors and shut down and turning them into something more meaningful on sys.stderr.

    The attached patch won't quite apply cleanly, since it predates r87172

    @ncoghlan
    Copy link
    Contributor Author

    Although it may be better to just raise a new, more meaningul, exception and let the runtime take care of ignoring it (since it happens in __del__)

    @ncoghlan
    Copy link
    Contributor Author

    Tidy ups committed in r87182. Shutdown problems now result in a slightly more meaningful message on stderr, a ResourceWarning is triggered when an implicit teardown occurs and the chances of an AttributeError due to an exception in __init__ are minimised.

    @bitdancer
    Copy link
    Member

    The tests are failing on windows:

    http://www.python.org/dev/buildbot/all/builders/x86%20XP-4%203.x/builds/3770/steps/test/logs/stdio

    ======================================================================
    ERROR: test_mkdtemp_failure (test.test_tempfile.test_TemporaryDirectory)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_tempfile.py", line 933, in test_mkdtemp_failure
        tempfile.TemporaryDirectory(prefix="[]<>?*!:")
      File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\tempfile.py", line 624, in __init__
        self.name = mkdtemp(suffix, prefix, dir)
      File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\tempfile.py", line 301, in mkdtemp
        _os.mkdir(file, 0o700)
    WindowsError: [Error 123] The filename, directory name, or volume label syntax is incorrect: 'c:\\docume~1\\db3l\\locals~1\\temp\\[]<>?*!:9ohp4a'

    ======================================================================
    FAIL: test_warnings_on_cleanup (test.test_tempfile.test_TemporaryDirectory)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_tempfile.py", line 1001, in test_warnings_on_cleanup
        self.assertIn(d.name,  message)
    AssertionError: 'c:\\docume~1\\db3l\\locals~1\\temp\\94ifn5\\ujobx5' not found in 'ERROR: TypeError("\'NoneType\' object is not callable",) while cleaning up <TemporaryDirectory \'c:\\\\docume~1\\\\db3l\\\\locals~1\\\\temp\\\\94ifn5\\\\ujobx5\'>\n'

    @bitdancer
    Copy link
    Member

    There's also a typo in the issue number in your commit message (10888 instead of 10188).

    @ncoghlan
    Copy link
    Contributor Author

    Sorry, I realised after I had logged off and gone to bed that I hadn't finished the last test. Fixed in r87204 with an approach that should exercise the relevant behaviour regardless of platform.

    The commit message has also been updated to refer to the correct issue.

    I'm actually going to close this now - the problem of misbehaviour due to modules being nulled out at shutdown is a more universal problem being tracked elsewhere, and I think the behaviour I just added is the best we can hope for until that is fixed.

    @ncoghlan ncoghlan added the type-bug An unexpected behavior, bug, or error label Dec 13, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Dec 13, 2010

    A test still fails under Windows:

    ======================================================================
    FAIL: test_warnings_on_cleanup (test.test_tempfile.test_TemporaryDirectory)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_tempfile.py", line 1003, in test_warnings_on_cleanup
        self.assertIn(d.name,  message)
    AssertionError: 'c:\\docume~1\\db3l\\locals~1\\temp\\c2h__k\\1kxw82' not found in 'ERROR: TypeError("\'NoneType\' object is not callable",) while cleaning up <TemporaryDirectory \'c:\\\\docume~1\\\\db3l\\\\locals~1\\\\temp\\\\c2h__k\\\\1kxw82\'>\n'

    @ncoghlan
    Copy link
    Contributor Author

    Ah, yes, I failed to account for the additional string escaping performed by the IO stream. I think I've been bitten by that before, but I always forget since I try to always use forward slashes for paths, even on Windows.

    r87212 modifies the test to ensure that failure mode is encountered on non-Windows systems as well and correctly reverses the escaping in the test when reading the message back.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants