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

Fix possible leaks in close methods #68053

Closed
serhiy-storchaka opened this issue Apr 4, 2015 · 7 comments
Closed

Fix possible leaks in close methods #68053

serhiy-storchaka opened this issue Apr 4, 2015 · 7 comments
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@serhiy-storchaka
Copy link
Member

BPO 23865
Nosy @vadmium, @serhiy-storchaka, @mitya57
Files
  • close.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 = None
    closed_at = <Date 2015-04-10.17:15:50.010>
    created_at = <Date 2015-04-04.11:13:00.166>
    labels = ['library', 'performance']
    title = 'Fix possible leaks in close methods'
    updated_at = <Date 2015-05-06.06:54:24.713>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-05-06.06:54:24.713>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-04-10.17:15:50.010>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2015-04-04.11:13:00.166>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['38829']
    hgrepos = []
    issue_num = 23865
    keywords = ['patch']
    message_count = 7.0
    messages = ['240063', '240245', '240255', '240289', '240413', '242580', '242645']
    nosy_count = 4.0
    nosy_names = ['python-dev', 'martin.panter', 'serhiy.storchaka', 'mitya57']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue23865'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @serhiy-storchaka
    Copy link
    Member Author

    Proposed patch fixes two related issues in a number of modules.

    1. close() methods sometimes release multiple resources. Every closing operation can fail, but it shouldn't prevent releasing other resources. See for example bpo-21802.

    2. close() should be idempotent. I.e. calling close() second times shouldn't have any effect. Even if close() failed, repeated call of close() (usually in __exit__(), in __del__(), or in finally block) shouldn't raise an exception.

    Many close() methods already satisfy these conditions, but not all.

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir performance Performance or resource usage labels Apr 4, 2015
    @vadmium
    Copy link
    Member

    vadmium commented Apr 8, 2015

    The change to aifc shouldn’t be needed, unless the underlying file object’s close() is not idempotent. And if that is the case, you’re fixing the bug in the wrong place :)

    Most of the other changes look good from a rough review, though I was only familiar enough with the gzip and HTTP modules to review them thoroughly. I left a few comments on Reitveld.

    @serhiy-storchaka
    Copy link
    Member Author

    Ah, yet one purpose of the patch is to make close() methods more robust when called at shutdown. Objects can be partially deconstructed at that time, attributes can be set to None to break reference loops. That is why I use None as signaling value and always check some attribute for None.

    The changes to aifc make sense. close() is called from __exit__(). When context manager is used in a generator, it creates a reference loop. So self.file is None in this case and self.file.close() will raise an AttributeError.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 8, 2015

    Okay, I think I get it. You are setting some attributes to None, in case they happen to be causing a reference cycle. When close() is called, they are no longer needed, so it might help to breaking potential reference cycles. I have often wondered why this was done. I always assumed it was a carry-over from old code relying on the garbage collector to automatically close files, etc.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 10, 2015

    New changeset f7ddec2e9e93 by Serhiy Storchaka in branch '2.7':
    Issue bpo-23865: close() methods in multiple modules now are idempotent and more
    https://hg.python.org/cpython/rev/f7ddec2e9e93

    New changeset e826940911c8 by Serhiy Storchaka in branch '3.4':
    Issue bpo-23865: close() methods in multiple modules now are idempotent and more
    https://hg.python.org/cpython/rev/e826940911c8

    New changeset 4ddec11b5faf by Serhiy Storchaka in branch 'default':
    Issue bpo-23865: close() methods in multiple modules now are idempotent and more
    https://hg.python.org/cpython/rev/4ddec11b5faf

    @mitya57
    Copy link
    Mannequin

    mitya57 mannequin commented May 4, 2015

    This broke docutils, see issue bpo-24125.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 6, 2015

    New changeset fe340c2a220e by Serhiy Storchaka in branch '2.7':
    Issue bpo-24125: Saved error's line and column numbers when an error is occured
    https://hg.python.org/cpython/rev/fe340c2a220e

    New changeset 7f8cd879687b by Serhiy Storchaka in branch '3.4':
    Issue bpo-24125: Saved error's line and column numbers when an error is occured
    https://hg.python.org/cpython/rev/7f8cd879687b

    New changeset 1fb83fa2cdef by Serhiy Storchaka in branch 'default':
    Issue bpo-24125: Saved error's line and column numbers when an error is occured
    https://hg.python.org/cpython/rev/1fb83fa2cdef

    @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
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants