Merge pull request #6879 from chrahunt/bugfix/dont-lock-selfcheck

Don't use lockfile to protect updates to selfcheck file.

news/6954.bugfix

@@ -0,0 +1 @@
+Don't use hardlinks for locking selfcheck state file.

src/pip/_internal/utils/filesystem.py

@@ -2,8 +2,26 @@ import os
 import os.path
 import shutil
 import stat
+from contextlib import contextmanager
+from tempfile import NamedTemporaryFile
+
+# NOTE: retrying is not annotated in typeshed as on 2017-07-17, which is
+#       why we ignore the type on this import.
+from pip._vendor.retrying import retry  # type: ignore
+from pip._vendor.six import PY2
 
 from pip._internal.utils.compat import get_path_uid
+from pip._internal.utils.misc import cast
+from pip._internal.utils.typing import MYPY_CHECK_RUNNING
+
+if MYPY_CHECK_RUNNING:
+    from typing import BinaryIO, Iterator
+
+    class NamedTemporaryFileResult(BinaryIO):
+        @property
+        def file(self):
+            # type: () -> BinaryIO
+            pass
 
 
 def check_path_owner(path):
@@ -59,3 +77,39 @@ def copy2_fixed(src, dest):
 def is_socket(path):
     # type: (str) -> bool
     return stat.S_ISSOCK(os.lstat(path).st_mode)
+
+
+@contextmanager
+def adjacent_tmp_file(path):
+    # type: (str) -> Iterator[NamedTemporaryFileResult]
+    """Given a path to a file, open a temp file next to it securely and ensure
+    it is written to disk after the context reaches its end.
+    """
+    with NamedTemporaryFile(
+        delete=False,
+        dir=os.path.dirname(path),
+        prefix=os.path.basename(path),
+        suffix='.tmp',
+    ) as f:
+        result = cast('NamedTemporaryFileResult', f)
+        try:
+            yield result
+        finally:
+            result.file.flush()
+            os.fsync(result.file.fileno())
+
+
+_replace_retry = retry(stop_max_delay=1000, wait_fixed=250)
+
+if PY2:
+    @_replace_retry
+    def replace(src, dest):
+        # type: (str, str) -> None
+        try:
+            os.rename(src, dest)
+        except OSError:
+            os.remove(dest)
+            os.rename(src, dest)
+
+else:
+    replace = _replace_retry(os.replace)

src/pip/_internal/utils/outdated.py

@@ -7,7 +7,7 @@ import logging
 import os.path
 import sys
 
-from pip._vendor import lockfile, pkg_resources
+from pip._vendor import pkg_resources
 from pip._vendor.packaging import version as packaging_version
 from pip._vendor.six import ensure_binary
 
@@ -15,7 +15,11 @@ from pip._internal.cli.cmdoptions import make_search_scope
 from pip._internal.index import PackageFinder
 from pip._internal.models.selection_prefs import SelectionPreferences
 from pip._internal.utils.compat import WINDOWS
-from pip._internal.utils.filesystem import check_path_owner
+from pip._internal.utils.filesystem import (
+    adjacent_tmp_file,
+    check_path_owner,
+    replace,
+)
 from pip._internal.utils.misc import ensure_dir, get_installed_version
 from pip._internal.utils.packaging import get_installer
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
@@ -86,12 +90,16 @@ class SelfCheckState(object):
 
         text = json.dumps(state, sort_keys=True, separators=(",", ":"))
 
-        # Attempt to write out our version check file
-        with lockfile.LockFile(self.statefile_path):
+        with adjacent_tmp_file(self.statefile_path) as f:
+            f.write(ensure_binary(text))
+
+        try:
             # Since we have a prefix-specific state file, we can just
             # overwrite whatever is there, no need to check.
-            with open(self.statefile_path, "w") as statefile:
-                statefile.write(text)
+            replace(f.name, self.statefile_path)
+        except OSError:
+            # Best effort.
+            pass
 
 
 def was_installed_by_pip(pkg):

tests/unit/test_unit_outdated.py

@@ -2,12 +2,11 @@ import datetime
 import json
 import os
 import sys
-from contextlib import contextmanager
 
 import freezegun
 import pretend
 import pytest
-from pip._vendor import lockfile, pkg_resources
+from pip._vendor import pkg_resources
 
 from pip._internal.index import InstallationCandidate
 from pip._internal.utils import outdated
@@ -162,49 +161,6 @@ def _get_statefile_path(cache_dir, key):
     )
 
 
-def test_self_check_state(monkeypatch, tmpdir):
-    CONTENT = '''{"key": "pip_prefix", "last_check": "1970-01-02T11:00:00Z",
-        "pypi_version": "1.0"}'''
-    fake_file = pretend.stub(
-        read=pretend.call_recorder(lambda: CONTENT),
-        write=pretend.call_recorder(lambda s: None),
-    )
-
-    @pretend.call_recorder
-    @contextmanager
-    def fake_open(filename, mode='r'):
-        yield fake_file
-
-    monkeypatch.setattr(outdated, 'open', fake_open, raising=False)
-
-    @pretend.call_recorder
-    @contextmanager
-    def fake_lock(filename):
-        yield
-
-    monkeypatch.setattr(outdated, "check_path_owner", lambda p: True)
-
-    monkeypatch.setattr(lockfile, 'LockFile', fake_lock)
-
-    cache_dir = tmpdir / 'cache_dir'
-    key = 'pip_prefix'
-    monkeypatch.setattr(sys, 'prefix', key)
-
-    state = SelfCheckState(cache_dir=cache_dir)
-    state.save('2.0', datetime.datetime.utcnow())
-
-    expected_path = _get_statefile_path(str(cache_dir), key)
-    assert fake_lock.calls == [pretend.call(expected_path)]
-
-    assert fake_open.calls == [
-        pretend.call(expected_path),
-        pretend.call(expected_path, 'w'),
-    ]
-
-    # json.dumps will call this a number of times
-    assert len(fake_file.write.calls)
-
-
 def test_self_check_state_no_cache_dir():
     state = SelfCheckState(cache_dir=False)
     assert state.state == {}