Merge pull request #194 from python/bugfix/bpo-41490

Release handle on zipfile when generating path in ZipReader

Author: jaraco

coverage.ini

@@ -2,14 +2,15 @@
 branch = true
 parallel = true
 omit =
-     setup*
+    setup*
     .tox/*/lib/python*/site-packages/*
     */tests/*.py
     */testing/*.py
     importlib_resources/_py${OMIT}.py
     importlib_resources/__init__.py
     importlib_resources/_compat.py
     importlib_resources/abc.py
+    *.zip*
 plugins =
      coverplug
 

docs/changelog.rst

@@ -2,6 +2,14 @@
  importlib_resources NEWS
 ==========================
 
+v3.1.0
+======
+
+* #110 and bpo-41490: ``path`` method is more aggressive about
+  releasing handles to zipfile objects early, enabling use-cases
+  like ``certifi`` to leave the context open but delete the underlying
+  zip file.
+
 v3.0.0
 ======
 

importlib_resources/_common.py

@@ -93,6 +93,7 @@ def _tempfile(reader, suffix=''):
     try:
         os.write(fd, reader())
         os.close(fd)
+        del reader
         yield Path(raw_path)
     finally:
         try:
@@ -102,14 +103,12 @@ def _tempfile(reader, suffix=''):
 
 
 @singledispatch
-@contextlib.contextmanager
 def as_file(path):
     """
     Given a Traversable object, return that object as a
     path on the local file system in a context manager.
     """
-    with _tempfile(path.read_bytes, suffix=path.name) as local:
-        yield local
+    return _tempfile(path.read_bytes, suffix=path.name)
 
 
 @as_file.register(Path)

importlib_resources/_py3.py

@@ -1,8 +1,9 @@
 import os
 import sys
+import io
 
 from . import _common
-from contextlib import contextmanager, suppress
+from contextlib import suppress
 from importlib.abc import ResourceLoader
 from io import BytesIO, TextIOWrapper
 from pathlib import Path
@@ -94,22 +95,26 @@ def path(
     """
     reader = _common.get_resource_reader(_common.get_package(package))
     return (
-        _path_from_reader(reader, resource)
+        _path_from_reader(reader, _common.normalize_path(resource))
         if reader else
         _common.as_file(
             _common.files(package).joinpath(_common.normalize_path(resource)))
         )
 
 
-@contextmanager
 def _path_from_reader(reader, resource):
-    norm_resource = _common.normalize_path(resource)
+    return _path_from_resource_path(reader, resource) or \
+        _path_from_open_resource(reader, resource)
+
+
+def _path_from_resource_path(reader, resource):
     with suppress(FileNotFoundError):
-        yield Path(reader.resource_path(norm_resource))
-        return
-    opener_reader = reader.open_resource(norm_resource)
-    with _common._tempfile(opener_reader.read, suffix=norm_resource) as res:
-        yield res
+        return Path(reader.resource_path(resource))
+
+
+def _path_from_open_resource(reader, resource):
+    saved = io.BytesIO(reader.open_resource(resource).read())
+    return _common._tempfile(saved.read, suffix=resource)
 
 
 def is_resource(package: Package, name: str) -> bool:

importlib_resources/readers.py

@@ -22,8 +22,8 @@ def files(self):
 class ZipReader(abc.TraversableResources):
     def __init__(self, loader, module):
         _, _, name = module.rpartition('.')
-        prefix = loader.prefix.replace('\\', '/') + name + '/'
-        self.path = ZipPath(loader.archive, prefix)
+        self.prefix = loader.prefix.replace('\\', '/') + name + '/'
+        self.archive = loader.archive
 
     def open_resource(self, resource):
         try:
@@ -38,4 +38,4 @@ def is_resource(self, path):
         return target.is_file() and target.exists()
 
     def files(self):
-        return self.path
+        return ZipPath(self.archive, self.prefix)

importlib_resources/tests/_compat.py

@@ -0,0 +1,30 @@
+try:
+    from test.support import import_helper  # type: ignore
+except ImportError:
+    try:
+        # Python 3.9 and earlier
+        class import_helper:  # type: ignore
+            from test.support import modules_setup, modules_cleanup
+    except ImportError:
+        from . import py27compat
+
+        class import_helper:  # type: ignore
+            modules_setup = staticmethod(py27compat.modules_setup)
+            modules_cleanup = staticmethod(py27compat.modules_cleanup)
+
+
+try:
+    from os import fspath  # type: ignore
+except ImportError:
+    # Python 3.5
+    fspath = str  # type: ignore
+
+
+try:
+    # Python 3.10
+    from test.support.os_helper import unlink
+except ImportError:
+    from test.support import unlink as _unlink
+
+    def unlink(target):
+        return _unlink(fspath(target))

importlib_resources/tests/py27compat.py

@@ -0,0 +1,23 @@
+import sys
+
+
+def modules_setup():
+    return sys.modules.copy(),
+
+
+def modules_cleanup(oldmodules):
+    # Encoders/decoders are registered permanently within the internal
+    # codec cache. If we destroy the corresponding modules their
+    # globals will be set to None which will trip up the cached functions.
+    encodings = [(k, v) for k, v in sys.modules.items()
+                 if k.startswith('encodings.')]
+    sys.modules.clear()
+    sys.modules.update(encodings)
+    # XXX: This kind of problem can affect more than just encodings.
+    # In particular extension modules (such as _ssl) don't cope
+    # with reloading properly. Really, test modules should be cleaning
+    # out the test specific modules they know they added (ala test_runpy)
+    # rather than relying on this function (as test_importhooks and test_pkg
+    # do currently). Implicitly imported *real* modules should be left alone
+    # (see issue 10556).
+    sys.modules.update(oldmodules)

importlib_resources/tests/test_resource.py

@@ -1,11 +1,15 @@
 import sys
 import unittest
 import importlib_resources as resources
+import uuid
+
+from importlib_resources._compat import Path
 
 from . import data01
 from . import zipdata01, zipdata02
 from . import util
 from importlib import import_module
+from ._compat import import_helper, unlink
 
 
 class ResourceTests:
@@ -166,5 +170,80 @@ def test_namespaces_cannot_have_resources(self):
             'importlib_resources.tests.data03.namespace', 'resource1.txt')
 
 
+class DeletingZipsTest(unittest.TestCase):
+    """Having accessed resources in a zip file should not keep an open
+    reference to the zip.
+    """
+    ZIP_MODULE = zipdata01
+
+    def setUp(self):
+        modules = import_helper.modules_setup()
+        self.addCleanup(import_helper.modules_cleanup, *modules)
+
+        data_path = Path(self.ZIP_MODULE.__file__)
+        data_dir = data_path.parent
+        self.source_zip_path = data_dir / 'ziptestdata.zip'
+        self.zip_path = Path('{}.zip'.format(uuid.uuid4())).absolute()
+        self.zip_path.write_bytes(self.source_zip_path.read_bytes())
+        sys.path.append(str(self.zip_path))
+        self.data = import_module('ziptestdata')
+
+    def tearDown(self):
+        try:
+            sys.path.remove(str(self.zip_path))
+        except ValueError:
+            pass
+
+        try:
+            del sys.path_importer_cache[str(self.zip_path)]
+            del sys.modules[self.data.__name__]
+        except KeyError:
+            pass
+
+        try:
+            unlink(self.zip_path)
+        except OSError:
+            # If the test fails, this will probably fail too
+            pass
+
+    def test_contents_does_not_keep_open(self):
+        c = resources.contents('ziptestdata')
+        self.zip_path.unlink()
+        del c
+
+    def test_is_resource_does_not_keep_open(self):
+        c = resources.is_resource('ziptestdata', 'binary.file')
+        self.zip_path.unlink()
+        del c
+
+    def test_is_resource_failure_does_not_keep_open(self):
+        c = resources.is_resource('ziptestdata', 'not-present')
+        self.zip_path.unlink()
+        del c
+
+    @unittest.skip("Desired but not supported.")
+    def test_path_does_not_keep_open(self):
+        c = resources.path('ziptestdata', 'binary.file')
+        self.zip_path.unlink()
+        del c
+
+    def test_entered_path_does_not_keep_open(self):
+        # This is what certifi does on import to make its bundle
+        # available for the process duration.
+        c = resources.path('ziptestdata', 'binary.file').__enter__()
+        self.zip_path.unlink()
+        del c
+
+    def test_read_binary_does_not_keep_open(self):
+        c = resources.read_binary('ziptestdata', 'binary.file')
+        self.zip_path.unlink()
+        del c
+
+    def test_read_text_does_not_keep_open(self):
+        c = resources.read_text('ziptestdata', 'utf-8.file', encoding='utf-8')
+        self.zip_path.unlink()
+        del c
+
+
 if __name__ == '__main__':
     unittest.main()

importlib_resources/tests/util.py

@@ -9,30 +9,7 @@
 from . import zipdata01
 from .._compat import ABC, Path, PurePath, FileNotFoundError
 from ..abc import ResourceReader
-
-try:
-    from test.support import modules_setup, modules_cleanup
-except ImportError:
-    # Python 2.7.
-    def modules_setup():
-        return sys.modules.copy(),
-
-    def modules_cleanup(oldmodules):
-        # Encoders/decoders are registered permanently within the internal
-        # codec cache. If we destroy the corresponding modules their
-        # globals will be set to None which will trip up the cached functions.
-        encodings = [(k, v) for k, v in sys.modules.items()
-                     if k.startswith('encodings.')]
-        sys.modules.clear()
-        sys.modules.update(encodings)
-        # XXX: This kind of problem can affect more than just encodings. In
-        # particular extension modules (such as _ssl) don't cope with reloading
-        # properly.  Really, test modules should be cleaning out the test
-        # specific modules they know they added (ala test_runpy) rather than
-        # relying on this function (as test_importhooks and test_pkg do
-        # currently).  Implicitly imported *real* modules should be left alone
-        # (see issue 10556).
-        sys.modules.update(oldmodules)
+from ._compat import import_helper
 
 
 try:
@@ -205,8 +182,8 @@ def tearDownClass(cls):
             pass
 
     def setUp(self):
-        modules = modules_setup()
-        self.addCleanup(modules_cleanup, *modules)
+        modules = import_helper.modules_setup()
+        self.addCleanup(import_helper.modules_cleanup, *modules)
 
 
 class ZipSetup(ZipSetupBase):

tox.ini

@@ -7,8 +7,8 @@ toxworkdir={env:TOX_WORK_DIR:.tox}
 
 [testenv]
 commands =
-    !cov,!diffcov: python -m unittest discover
-    cov,diffcov: python -m coverage run {[coverage]rc} -m unittest discover {posargs}
+    !cov,!diffcov: python -m unittest {posargs:discover}
+    cov,diffcov: python -m coverage run {[coverage]rc} -m unittest {posargs:discover}
     cov,diffcov: python -m coverage combine {[coverage]rc}
     cov: python -m coverage html {[coverage]rc}
     cov: python -m coverage xml {[coverage]rc}