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.NamedTemporaryFile can close the file too early, if not assigning to a variable #63079

Closed
jortbloem mannequin opened this issue Aug 29, 2013 · 22 comments
Closed
Assignees
Labels
stdlib Python modules in the Lib dir topic-IO type-bug An unexpected behavior, bug, or error

Comments

@jortbloem
Copy link
Mannequin

jortbloem mannequin commented Aug 29, 2013

BPO 18879
Nosy @birkenfeld, @ncoghlan, @pitrou, @vstinner, @bitdancer, @meadori, @socketpair, @vadmium, @serhiy-storchaka, @jstasiak
Files
  • tempfile.patch
  • tempfile_simplify.patch
  • tempfile_wrap_methods.patch
  • tempfile_lifetime.patch
  • 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/pitrou'
    closed_at = <Date 2013-12-21.21:21:12.772>
    created_at = <Date 2013-08-29.20:25:43.325>
    labels = ['type-bug', 'library', 'expert-IO']
    title = 'tempfile.NamedTemporaryFile can close the file too early, if not assigning to a variable'
    updated_at = <Date 2014-05-26.06:27:57.890>
    user = 'https://bugs.python.org/jortbloem'

    bugs.python.org fields:

    activity = <Date 2014-05-26.06:27:57.890>
    actor = 'socketpair'
    assignee = 'pitrou'
    closed = True
    closed_date = <Date 2013-12-21.21:21:12.772>
    closer = 'pitrou'
    components = ['Library (Lib)', 'IO']
    creation = <Date 2013-08-29.20:25:43.325>
    creator = 'jort.bloem'
    dependencies = []
    files = ['31598', '31603', '33153', '33252']
    hgrepos = []
    issue_num = 18879
    keywords = ['patch']
    message_count = 22.0
    messages = ['196485', '196496', '196501', '196504', '196507', '196952', '196985', '196991', '196992', '197016', '197019', '204723', '206257', '206258', '206554', '206776', '206778', '206780', '206781', '206782', '216506', '219138']
    nosy_count = 15.0
    nosy_names = ['georg.brandl', 'ncoghlan', 'ctheune', 'pitrou', 'vstinner', 'Arfrever', 'r.david.murray', 'meador.inge', 'socketpair', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'jort.bloem', 'Zden\xc4\x9bk.Pavlas', 'jstasiak']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue18879'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @jortbloem
    Copy link
    Mannequin Author

    jortbloem mannequin commented Aug 29, 2013

    Issue occurs in the following code: tempfile.NamedTemporaryFile(dir=".",delete=False).write("hello")

    The NamedTemporaryFile is deleted before the write is complete; deleting the object closes the file, so the write fails.

    @jortbloem jortbloem mannequin added the type-bug An unexpected behavior, bug, or error label Aug 29, 2013
    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir topic-IO labels Aug 29, 2013
    @jortbloem
    Copy link
    Mannequin Author

    jortbloem mannequin commented Aug 29, 2013

    Sorry, my python-foo is not quite up to providing a solution. A partial solution would be for the file not to be closed at NamedTemporaryFile destruction, if delete==False. If delete==True, then there is no point writing to a file (though it shouldn't raise an exception, either).

    Alternatively, on a unix based system, the file could be deleted, but left open. Read & write could continue as per normal, and the file would be closed automatically later.

    @ncoghlan
    Copy link
    Contributor

    The method call should keep the file object alive until it completes (due to the self reference). What behaviour prompted the conclusion that it is being closed early?

    If the symptom is that the data isn't being written to disk, we may have a missing flush() call somewhere causing a race condition.

    @meadori
    Copy link
    Member

    meadori commented Aug 29, 2013

    Confirmed in trunk:

    >>> tempfile.NamedTemporaryFile(dir=".",delete=False).write(b"hello")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: write to closed file

    @meadori
    Copy link
    Member

    meadori commented Aug 30, 2013

    'tempfile.NamedTemporaryFile' returns an instance of '_TemporaryFileWrapper' that wraps the created file object. The wrapper object implements '__getattr__' and we end up with a situation like the following simplified example where the wrapper gets destroyed before the looked up method is called:

    $ cat test.py 
    class S(object):
       def __init__(self):
          print("S.__init__")   def __del__(self):      print("S.__del__")
       def foo(self):
          print("S.foo")
    class T(object):
       def __init__(self, s):
          self.s = s
          print("T.__init__")
       def __getattr__(self, name):
            s = self.__dict__['s']
            a = getattr(s, name)
            print("__getattr__('%s')" % name)
            if not isinstance(a, int):
                setattr(self, name, a)
            return a
       def __del__(self):
          print("T.__del__")
    T(S()).foo()
    $ ./python test.py 
    S.__init__
    T.__init__
    __getattr__('foo')
    T.__del__
    S.foo
    S.__del__

    @jortbloem
    Copy link
    Mannequin Author

    jortbloem mannequin commented Sep 4, 2013

    I am only new to Python, but...

    Having looked at the code, I am surprised that the _TemporaryFileWrapper is not a subclass of "file". Surely that would be the "python" way, and would solve this problem, too? __getattr__ would no longer be needed. The opening of the file is within the library, so _os.open() could be replaced with file()... It would require rewriting/reorganising chunks of this code...

    @jstasiak
    Copy link
    Mannequin

    jstasiak mannequin commented Sep 5, 2013

    I don't see an obvious way of solving that. One thing I could think of is creating a wrapper for file method being returned from __getattr__ that holds reference to _TemporaryFileWrapper instance until the method gets called, please find a patch attached.

    @serhiy-storchaka
    Copy link
    Member

    Yet one way is just remove __del__() and monkey-patch underlied file.

    @meadori
    Copy link
    Member

    meadori commented Sep 5, 2013

    I was experimenting with something similar to what Jakub has. Rebinding the call the the wrapper seems like a simple and clean way to do it. Although, I wasn't absolutely sure whether this approach covers all cases.

    @meadori
    Copy link
    Member

    meadori commented Sep 5, 2013

    The monkey-patched version breaks the auto-close on __del__ feature:

    [meadori@li589-207 cpython]$ ./python 
    Python 3.4.0a1+ (default:c41c68a18bb6, Sep  5 2013, 17:51:03) 
    [GCC 4.7.2 20120921 (Red Hat 4.7.2-2)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import tempfile
    >>> tempfile.NamedTemporaryFile(dir=".",delete=False).write(b"hello")
    __main__:1: ResourceWarning: unclosed file <_io.BufferedRandom name=4>
    5

    Also run the test suite with '-R :' and you will see tons of resource warnings.

    Breaking auto-close is because now unlinking *and* closing are being guarded by 'delete'. Even if that is fixed you still run into resource leak problems. Looks like the monkey-patching introduces a reference cycle.

    The wrapper binding approach works fine in all the tests I did.

    @serhiy-storchaka
    Copy link
    Member

    The monkey-patched version breaks the auto-close on __del__ feature:

    Files were closed. Just with a warning. I consider this as an enhancement. If you don't close file explicitly you will get a leak on non-refcounted Python implementations. It's true for both ordinal files so tempfiles. Perhaps we should add a warning in any __del__ method (even if we will commit Jakub's patch, this way LGTM too).

    @ZdenkPavlas
    Copy link
    Mannequin

    ZdenkPavlas mannequin commented Nov 29, 2013

    Hit this issue too, but with the "read" method. I think that providing a custom read() and write() methods in the wrapper class that override __getattr__ is a sufficient solution. These are the attributes most likely to be used as the "tail".

    @serhiy-storchaka
    Copy link
    Member

    Jakub's patch has a bug:

    >>> import tempfile
    >>> f = tempfile.NamedTemporaryFile(dir=".",delete=False)
    >>> write = f.write
    >>> write
    <function BufferedRandom.write at 0xb716b4f4>
    >>> write2 = f.write
    >>> write2
    <built-in method write of _io.BufferedRandom object at 0xb721deac>
    >>> del f
    >>> write(b'foo')
    3
    >>> del write
    >>> write2(b'bar')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: write to closed file

    @serhiy-storchaka
    Copy link
    Member

    Here is fixed Jakub's patch.

    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 15, 2013
    @serhiy-storchaka
    Copy link
    Member

    Unfortunately last patch causes resource leak on 3.3+.

    $ ./python -Wall -m test.regrtest test_tempfile
    [1/1] test_tempfile
    1 test OK.
    sys:1: ResourceWarning: gc: 5 uncollectable objects at shutdown; use gc.set_debug(gc.DEBUG_UNCOLLECTABLE) to list them

    @pitrou
    Copy link
    Member

    pitrou commented Dec 21, 2013

    Here is a proper patch (for 3.3). Untested under Windows, but should work.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 21, 2013

    Oops, removed two debug lines.

    @serhiy-storchaka
    Copy link
    Member

    LGTM. Thank you Antoine.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 21, 2013

    New changeset f3b7a76fb778 by Antoine Pitrou in branch '3.3':
    Issue bpo-18879: When a method is looked up on a temporary file, avoid closing the file before the method is possibly called.
    http://hg.python.org/cpython/rev/f3b7a76fb778

    New changeset d68ab2eb7a77 by Antoine Pitrou in branch 'default':
    Issue bpo-18879: When a method is looked up on a temporary file, avoid closing the file before the method is possibly called.
    http://hg.python.org/cpython/rev/d68ab2eb7a77

    @pitrou
    Copy link
    Member

    pitrou commented Dec 21, 2013

    Fixed in 3.3 and 3.4. I think 2.7 is irrelevant at this point.

    @pitrou pitrou closed this as completed Dec 21, 2013
    @ctheune
    Copy link
    Mannequin

    ctheune mannequin commented Apr 16, 2014

    bpo-15002 uses this patch to fix a similar wrapping problem in urllib.

    Also, this affects 2.7 as well and bpo-15002 does report the problem for 2.7. I'd like to get this fix backported. Would that be OK?

    @ctheune ctheune mannequin changed the title tempfile.NamedTemporaryFile can close the file too early, if not assigning to a variable tempfile.NamedTemporaryFile can close the file too early, if not assigning to a variablwe Apr 16, 2014
    @Arfrever Arfrever mannequin changed the title tempfile.NamedTemporaryFile can close the file too early, if not assigning to a variablwe tempfile.NamedTemporaryFile can close the file too early, if not assigning to a variable Apr 17, 2014
    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented May 26, 2014

    Is bpo-21579 fixed in that bug? too many letters..sorry...

    @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
    stdlib Python modules in the Lib dir topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants