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