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

ZlibStorage.iterator() leaks the underlying iterator #4

Closed
jamadden opened this issue Jan 4, 2017 · 0 comments
Closed

ZlibStorage.iterator() leaks the underlying iterator #4

jamadden opened this issue Jan 4, 2017 · 0 comments

Comments

@jamadden
Copy link
Member

jamadden commented Jan 4, 2017

ZlibStorage.iterator is implemented as a generator, calling the underlying storage's iterator method. Because it's a generator, it doesn't expose the close method (if any) of the underlying iterator. This can lead to resources being leaked; at the very least, it's dependent on the CPython garbage collector for timely cleanup (which can be very tricky with generators).

For example, in RelStorage, the zodbconvert utility uses the storage's iterator to see if it has data. The tests methods that use zodbconvert with a zlibstorage wrapping a FileStorage produce this ResourceWarning (under 3.6 with tracemalloc to get a stacktrace):

//relstorage/relstorage/zodbconvert.py:52: ResourceWarning: unclosed file <_io.BufferedReader name='/var/folders/y5/x7pvzk651c3dqkllbxd1jd280000gn/T/tmpy9788r9h'>
  i.close()
Object allocated at (most recent call first):
  File "//lib/python3.6/site-packages/ZODB/FileStorage/FileStorage.py", lineno 1790
    file = open(filename, 'rb')
  File "//lib/python3.6/site-packages/ZODB/FileStorage/FileStorage.py", lineno 1371
    return FileIterator(self._file_name, start, stop)
  File "//lib/python3.6/site-packages/zc/zlibstorage/__init__.py", lineno 98
    for t in self.base.iterator(start, stop):
  File "//relstorage/relstorage/zodbconvert.py", lineno 46
    next(i)
  File "//relstorage/relstorage/zodbconvert.py", lineno 117
    if not storage_has_data(destination):

close isn't part of the storage iterator interface, AFAICS, but FileStorage's iterator provides a close method, as does the iterator provided by RelStorage. ZEO's doesn't, instead relying on a manual-ish _iterator_gc method (but maybe it could benefit from one?).

I'm happy to provide a PR to address this, possibly by making the returned iterator a new class that proxies unknown attributes to the underlying iterator.

EDIT: Or we could just use a try/finally block. But if memory serves, it's the try/finally blocks in a generator that lead to GC trouble...So we could catch GeneratorExit instead, but that's actually discouraged in the release notes.

@jamadden jamadden self-assigned this Jan 5, 2017
jamadden added a commit that referenced this issue Jan 5, 2017
Fixes #4.

Also add PyPy support.

Modernize the .travis.yml: sudo false, pip caching, same testrunner as
tox.ini.

Pin ZODB/ZEO to old versions because ServerZlibStorage is broken for
ZODB 5. Will address in a subsequent PR.
jamadden added a commit that referenced this issue Jan 5, 2017
Fixes #4.

Also add PyPy support.

Modernize the .travis.yml: sudo false, pip caching, same testrunner as
tox.ini.

Pin ZODB/ZEO to old versions because ServerZlibStorage is broken for
ZODB 5. Will address in a subsequent PR.
jamadden added a commit that referenced this issue Jan 5, 2017
Fixes #4.

Also add PyPy support.

Modernize the .travis.yml: sudo false, pip caching, same testrunner as
tox.ini.

Pin ZODB/ZEO to old versions because ServerZlibStorage is broken for
ZODB 5. Will address in a subsequent PR.
jamadden added a commit that referenced this issue Jan 5, 2017
Fixes #4.

Also add PyPy support.

Modernize the .travis.yml: sudo false, pip caching, same testrunner as
tox.ini.

Pin ZODB/ZEO to old versions because ServerZlibStorage is broken for
ZODB 5. Will address in a subsequent PR.
jamadden added a commit that referenced this issue Jan 5, 2017
Fixes #4.

Also add PyPy support.

Modernize the .travis.yml: sudo false, pip caching, same testrunner as
tox.ini.

Pin ZODB/ZEO to old versions because ServerZlibStorage is broken for
ZODB 5. Will address in a subsequent PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant