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

Add the close method for ElementTree.iterparse() object #69893

Closed
serhiy-storchaka opened this issue Nov 23, 2015 · 10 comments
Closed

Add the close method for ElementTree.iterparse() object #69893

serhiy-storchaka opened this issue Nov 23, 2015 · 10 comments
Assignees
Labels
3.8 only security fixes stdlib Python modules in the Lib dir topic-XML type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Nov 23, 2015

BPO 25707
Nosy @scoder, @serhiy-storchaka, @Vgr255, @furkanonder, @jacobtylerwalls
PRs
  • bpo-43292: Fix file leak in ET.iterparse() when not exhausted #31696
  • Dependencies
  • bpo-25638: Verify the etree_parse and etree_iterparse benchmarks are working appropriately
  • 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 = None
    created_at = <Date 2015-11-23.13:57:25.201>
    labels = ['3.8', 'library', 'performance']
    title = 'Add the close method for ElementTree.iterparse() object'
    updated_at = <Date 2022-03-05.15:26:50.497>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2022-03-05.15:26:50.497>
    actor = 'jacobtylerwalls'
    assignee = 'serhiy.storchaka'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2015-11-23.13:57:25.201>
    creator = 'serhiy.storchaka'
    dependencies = ['25638']
    files = []
    hgrepos = []
    issue_num = 25707
    keywords = ['patch']
    message_count = 8.0
    messages = ['255159', '255164', '255171', '255173', '341003', '341253', '341258', '368308']
    nosy_count = 5.0
    nosy_names = ['scoder', 'serhiy.storchaka', 'abarry', 'furkanonder', 'jacobtylerwalls']
    pr_nums = ['31696']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue25707'
    versions = ['Python 3.8']

    Linked PRs

    @serhiy-storchaka
    Copy link
    Member Author

    If ElementTree.iterparse() is called with file names, it opens a file. When resulting iterator is not exhausted, the file lefts not closed.

    >>> import xml.etree.ElementTree as ET
    >>> import gc
    >>> ET.iterparse('/dev/null')
    <xml.etree.ElementTree._IterParseIterator object at 0xb6f9e38c>
    >>> gc.collect()
    __main__:1: ResourceWarning: unclosed file <_io.BufferedReader name='/dev/null'>
    34

    Martin Panter proposed in bpo-25688 to add an explicit way to clean it up, like a generator.close() method.

    @serhiy-storchaka serhiy-storchaka self-assigned this Nov 23, 2015
    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir performance Performance or resource usage labels Nov 23, 2015
    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Nov 23, 2015

    I am unable to reproduce the issue on Windows 7 with 3.5.0; I have tried opening a small (non-empty) text. Here's the result:

    >>> import xml.etree.ElementTree as ET
    >>> import gc
    >>> ET.iterparse("E:/New.txt")
    <xml.etree.ElementTree._IterParseIterator object at 0x0023ABB0>
    >>> gc.collect()
    59

    @serhiy-storchaka
    Copy link
    Member Author

    You have to enable deprecation warnings. Run the interpreter with the -Wa option.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Nov 23, 2015

    Oh, my bad. Ignore my last message, behaviour is identical then. Thanks for clearing that up.

    @scoder
    Copy link
    Contributor

    scoder commented Apr 27, 2019

    I don't think there is a need for a close() method. Instead, the iterator should close the file first thing when it's done with it, but only if it owns it. Therefore, the fix in bpo-25688 seems correct.

    Closing can also be done explicitly in a finaliser of the iterator, if implicit closing via decref is too lax.

    @scoder scoder added the 3.8 only security fixes label Apr 27, 2019
    @serhiy-storchaka
    Copy link
    Member Author

    Implicit closing an exhausted iterator helps only the iterator is iterated to the end. If the iteration has been stopped before the end, we get a leak of the file descriptor. Closing the file descriptor in the finalizer can be deferred to undefined term, especially in implementations without reference counting. Since file descriptors are limited resource, this can cause troubles in real programs.

    Reasons for close() in iterparse objects are the same as for close in files and generators.

    Maybe we will need to implement the full generator protocol (send() and throw()) in the iterparse objects, but currently I do not know use cases for this.

    @scoder
    Copy link
    Contributor

    scoder commented May 2, 2019

    Ok, I think it's reasonable to make the resource management explicit for the specific case of letting iterparse() open the file. That suggests that there should also be context manager support, given that safe usages would often involve a try-finally.

    Since it might not always be obvious for users when they need to close the iterator or not, I would also suggest to not let it raise an error on a double-close, i.e. if .close() was already called or the iterator was already exhausted (and the file closed automatically), calling .close() should just do nothing.

    @furkanonder
    Copy link
    Mannequin

    furkanonder mannequin commented May 6, 2020

    Python 3.8.2 (default, Apr  8 2020, 14:31:25) 
    [GCC 9.3.0] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import xml.etree.ElementTree as ET
    >>> import gc
    >>> ET.iterparse('/dev/null')
    <xml.etree.ElementTree.iterparse.<locals>.IterParseIterator object at 0x7fb96f679d00>
    >>> gc.collect()
    34

    The warning(main:1: ResourceWarning: unclosed file <_io.BufferedReader name='/dev/null'>) is no longer available in python3.8.2

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @Prometheus3375
    Copy link
    Contributor

    I recently did a fully compatible implementation that solves an issue discussed here:

    class iterparse:
        __slots__ = '_source', '_opened', 'root', '_next'
    
        def __init__(self, /, source, events = None, parser = None):
            # If source cannot be opened,
            # an error is emitted and object is moved to gc,
            # gc calls __del__ which calls close().
            # opened flag must be set to false before opening,
            # so close() won't emit AttributeError.
            self._opened = False
            if hasattr(source, 'read'):
                self._source = source
            else:
                self._source = open(source, 'rb')
                self._opened = True
    
            self.root = None
            self._next = self._iterator(XMLPullParser(events=events, _parser=parser)).__next__
    
        def __iter__(self, /):
            return self
    
        def _iterator(self, parser: XMLPullParser, /):
            source = self._source
            try:
                data = source.read(16 * 1024)
                while data:
                    parser.feed(data)
                    yield from parser.read_events()
                    data = source.read(16 * 1024)
    
                root = parser._close_and_return_root()
                yield from parser.read_events()  # is it necessary?
                self.root = root
            finally:
                self.close()
    
        def __next__(self, /):
            return self._next()
    
        def close(self, /):
            if self._opened:
                self._source.close()
                self._opened = False
    
        def __del__(self, /):
            self.close()

    Features:

    • Fully compatible with current implementation.
    • According to my tests, implementation above has faster creation time (no new function and class each time), but a bit slower traverse time.
    • Has close method that does not raise an exception on double-close as @scoder asked (file descriptors also do not raise an error in such cases).
    • If there is no more references to iterparse object, it is destroyed closing opened fd.

    Benchmarks:
    I used this XML file for testing.

    def test_creation(file: str, impls: list[type], /):
        code = f'iterparse({file!r})'
    
        return tuple(
            repeat(
                code,
                repeat=50,
                number=1000,
                globals=dict(iterparse=t),
                )
            for t in impls
            )
    
    
    def test_traverse(file: str, impls: list[type], /):
        setup = f'it = iterparse({file!r})'
        code = f'for _ in it: pass'
    
        return tuple(
            repeat(
                code,
                setup=setup,
                repeat=50,
                number=10,
                globals=dict(iterparse=t),
                )
            for t in impls
            )
    
    
    def main():
        file_path = '20220628-FULL-1_1(xsd).xml'
        impls = [iterparse_new, iterparse_old]
    
        creation = test_creation(file_path, impls)
        for i, time in enumerate(creation):
            print(f'Creation time of {impls[i].__name__}:', min(time))
    
        print()
    
        traverse = test_traverse(file_path, impls)
        for i, time in enumerate(traverse):
            print(f'Traverse time of {impls[i].__name__}:', min(time))
    
    
    if __name__ == '__main__':
        main()
    Creation time of iterparse_new: 0.08623739999984537
    Creation time of iterparse: 0.10833569999977044
    
    Traverse time of iterparse_new: 0.3242015000005267
    Traverse time of iterparse: 0.3198846000004778
    

    @iritkatriel iritkatriel added type-bug An unexpected behavior, bug, or error and removed performance Performance or resource usage labels Aug 17, 2022
    namdre added a commit to eclipse-sumo/sumo that referenced this issue Mar 22, 2023
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jan 24, 2024
    @serhiy-storchaka
    Copy link
    Member Author

    Recent improvements (#101438) made explicit close() not needed in CPython. But it can still be useful in alternative implementations like PyPy, and maybe in future Python versions, so it is worth to add it.

    aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
    fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir topic-XML type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants