Merge pull request #3293 from pypa/bugfix/3289

Fix hash caching race condition

news/3289.bugfix.rst

@@ -0,0 +1 @@
+Fixed a race condition in hash resolution for dependencies for certain dependencies with missing cache entries or fresh Pipenv installs.

pipenv/utils.py

@@ -19,26 +19,13 @@
 from first import first
 from vistir.misc import fs_str
 
-six.add_move(six.MovedAttribute("Mapping", "collections", "collections.abc"))
-six.add_move(six.MovedAttribute("Sequence", "collections", "collections.abc"))
-from six.moves import Mapping, Sequence
+six.add_move(six.MovedAttribute("Mapping", "collections", "collections.abc"))  # noqa
+six.add_move(six.MovedAttribute("Sequence", "collections", "collections.abc"))  # noqa
+six.add_move(six.MovedAttribute("Set", "collections", "collections.abc"))  # noqa
+from six.moves import Mapping, Sequence, Set
 
 from vistir.compat import ResourceWarning
 
-try:
-    from weakref import finalize
-except ImportError:
-    try:
-        from .vendor.backports.weakref import finalize
-    except ImportError:
-
-        class finalize(object):
-            def __init__(self, *args, **kwargs):
-                logging.warn("weakref.finalize unavailable, not cleaning...")
-
-            def detach(self):
-                return False
-
 
 logging.basicConfig(level=logging.ERROR)
 
@@ -411,40 +398,55 @@ def resolve(self):
             self.resolved_tree.update(results)
             return self.resolved_tree
 
-    def resolve_hashes(self):
-        def _should_include_hash(ireq):
-            from pipenv.vendor.vistir.compat import Path, to_native_string
-            from pipenv.vendor.vistir.path import url_to_path
+    @staticmethod
+    def _should_include_hash(ireq):
+        from pipenv.vendor.vistir.compat import Path, to_native_string
+        from pipenv.vendor.vistir.path import url_to_path
 
-            # We can only hash artifacts.
-            try:
-                if not ireq.link.is_artifact:
-                    return False
-            except AttributeError:
+        # We can only hash artifacts.
+        try:
+            if not ireq.link.is_artifact:
                 return False
+        except AttributeError:
+            return False
 
-            # But we don't want normal pypi artifcats since the normal resolver
-            # handles those
-            if is_pypi_url(ireq.link.url):
-                return False
+        # But we don't want normal pypi artifcats since the normal resolver
+        # handles those
+        if is_pypi_url(ireq.link.url):
+            return False
 
-            # We also don't want to try to hash directories as this will fail
-            # as these are editable deps and are not hashable.
-            if (ireq.link.scheme == "file" and
-                    Path(to_native_string(url_to_path(ireq.link.url))).is_dir()):
-                return False
-            return True
+        # We also don't want to try to hash directories as this will fail
+        # as these are editable deps and are not hashable.
+        if (ireq.link.scheme == "file" and
+                Path(to_native_string(url_to_path(ireq.link.url))).is_dir()):
+            return False
+        return True
 
+    def resolve_hashes(self):
         if self.results is not None:
             resolved_hashes = self.resolver.resolve_hashes(self.results)
             for ireq, ireq_hashes in resolved_hashes.items():
+                # We _ALWAYS MUST PRIORITIZE_ the inclusion of hashes from local sources
+                # PLEASE *DO NOT MODIFY THIS* TO CHECK WHETHER AN IREQ ALREADY HAS A HASH
+                # RESOLVED. The resolver will pull hashes from PyPI and only from PyPI.
+                # The entire purpose of this approach is to include missing hashes.
+                # This fixes a race condition in resolution for missing dependency caches
+                # see pypa/pipenv#3289
+                if self._should_include_hash(ireq) and (
+                    not ireq_hashes or ireq.link.scheme == "file"
+                ):
+                    if not ireq_hashes:
+                        ireq_hashes = set()
+                    new_hashes = self.resolver.repository._hash_cache.get_hash(ireq.link)
+                    add_to_set(ireq_hashes, new_hashes)
+                else:
+                    ireq_hashes = set(ireq_hashes)
+                # The _ONLY CASE_ where we flat out set the value is if it isn't present
+                # It's a set, so otherwise we *always* need to do a union update
                 if ireq not in self.hashes:
-                    if _should_include_hash(ireq):
-                        self.hashes[ireq] = [
-                            self.resolver.repository._hash_cache.get_hash(ireq.link)
-                        ]
-                    else:
-                        self.hashes[ireq] = ireq_hashes
+                    self.hashes[ireq] = ireq_hashes
+                else:
+                    self.hashes[ireq] |= ireq_hashes
             return self.hashes
 
 
@@ -1485,3 +1487,15 @@ def sys_version(version_tuple):
     sys.version_info = version_tuple
     yield
     sys.version_info = old_version
+
+
+def add_to_set(original_set, element):
+    """Given a set and some arbitrary element, add the element(s) to the set"""
+    if not element:
+        return original_set
+    if isinstance(element, Set):
+        original_set |= element
+    elif isinstance(element, (list, tuple)):
+        original_set |= set(element)
+    else:
+        original_set.add(element)

tests/integration/test_lock.py

@@ -1,5 +1,6 @@
 import pytest
 import os
+import sys
 
 from pipenv.utils import temp_environ
 
@@ -517,3 +518,25 @@ def test_lock_no_warnings(PipenvInstance, pypi):
         assert "Warning" in c.err
         assert "Warning" not in c.out
         assert "hello" in c.out
+
+
+@pytest.mark.lock
+@pytest.mark.install
+@pytest.mark.skipif(sys.version_info >= (3, 5), reason="scandir doesn't get installed on python 3.5+")
+def test_lock_missing_cache_entries_gets_all_hashes(monkeypatch, PipenvInstance, pypi, tmpdir):
+    """
+    Test locking pathlib2 on python2.7 which needs `scandir`, but fails to resolve when
+    using a fresh dependency cache.
+    """
+
+    with monkeypatch.context() as m:
+        monkeypatch.setattr("pipenv.patched.piptools.locations.CACHE_DIR", tmpdir.strpath)
+        with PipenvInstance(pypi=pypi, chdir=True) as p:
+            p._pipfile.add("pathlib2", "*")
+            assert "pathlib2" in p.pipfile["packages"]
+            c = p.pipenv("install")
+            assert c.return_code == 0, c.err
+            assert "pathlib2" in p.lockfile["default"]
+            assert "scandir" in p.lockfile["default"]
+            assert isinstance(p.lockfile["default"]["scandir"]["hashes"], list)
+            assert len(p.lockfile["default"]["scandir"]["hashes"]) > 1