From 1dcce1d8b0ad2a8aa56bf51ed30de2466fd55acc Mon Sep 17 00:00:00 2001
From: Ruslan Kuprieiev <ruslan@iterative.ai>
Date: Mon, 6 Jan 2020 22:32:08 +0200
Subject: [PATCH] rwlock: check that .dvc/lock is locked

Mostly a sanity check, but we did have a potential bug in `destroy`,
whcih this PR fixes along the way.

Kudos @shcheklein for catching this one.
---
 dvc/lock.py              |  4 ++++
 dvc/repo/destroy.py      | 13 ++++++++++---
 dvc/stage.py             |  2 ++
 tests/func/test_repro.py |  2 +-
 tests/func/test_run.py   |  3 ++-
 tests/func/test_stage.py |  2 +-
 tests/unit/test_stage.py |  5 +++--
 7 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/dvc/lock.py b/dvc/lock.py
index ed2f2e21a1..b9c5b951d1 100644
--- a/dvc/lock.py
+++ b/dvc/lock.py
@@ -68,6 +68,10 @@ def unlock(self):
         self._lock.close()
         self._lock = None
 
+    @property
+    def is_locked(self):
+        return bool(self._lock)
+
     def __enter__(self):
         self.lock()
 
diff --git a/dvc/repo/destroy.py b/dvc/repo/destroy.py
index 0f92728541..c180778beb 100644
--- a/dvc/repo/destroy.py
+++ b/dvc/repo/destroy.py
@@ -1,8 +1,15 @@
 from dvc.utils.fs import remove
+from . import locked
 
 
-def destroy(self):
-    for stage in self.stages:
+@locked
+def _destroy_stages(repo):
+    for stage in repo.stages:
         stage.remove(remove_outs=False)
 
-    remove(self.dvc_dir)
+
+# NOTE: not locking `destroy`, as `remove` will need to delete `.dvc` dir,
+# which will cause issues on Windows, as `.dvc/lock` will be busy.
+def destroy(repo):
+    _destroy_stages(repo)
+    remove(repo.dvc_dir)
diff --git a/dvc/stage.py b/dvc/stage.py
index 1a94eed5f5..55e53bfb94 100644
--- a/dvc/stage.py
+++ b/dvc/stage.py
@@ -143,6 +143,8 @@ def rwlocked(call, read=None, write=None):
 
     stage = call._args[0]
 
+    assert stage.repo.lock.is_locked
+
     def _chain(names):
         return [
             item.path_info
diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py
index 09cf61ffeb..e7ea6e7356 100644
--- a/tests/func/test_repro.py
+++ b/tests/func/test_repro.py
@@ -841,7 +841,7 @@ def check_already_cached(self, stage):
 
         patch_run = patch.object(stage, "_run", wraps=stage._run)
 
-        with self.dvc.state:
+        with self.dvc.lock, self.dvc.state:
             with patch_download as mock_download:
                 with patch_checkout as mock_checkout:
                     with patch_run as mock_run:
diff --git a/tests/func/test_run.py b/tests/func/test_run.py
index 5d43b5083a..46bf0f1117 100644
--- a/tests/func/test_run.py
+++ b/tests/func/test_run.py
@@ -655,7 +655,8 @@ def test_run_deterministic_overwrite(deterministic_run):
 
 
 def test_run_deterministic_callback(deterministic_run):
-    deterministic_run.stage.remove()
+    with deterministic_run.stage.repo.lock:
+        deterministic_run.stage.remove()
     deterministic_run.deps = []
     deterministic_run.run()
     with mock.patch("dvc.prompt.confirm", return_value=True):
diff --git a/tests/func/test_stage.py b/tests/func/test_stage.py
index b87a32d76f..ec0fbe24af 100644
--- a/tests/func/test_stage.py
+++ b/tests/func/test_stage.py
@@ -108,7 +108,7 @@ def test_ignored_in_checksum(self):
         d = load_stage_file(stage.relpath)
         self.assertNotIn(Stage.PARAM_WDIR, d.keys())
 
-        with self.dvc.state:
+        with self.dvc.lock, self.dvc.state:
             stage = Stage.load(self.dvc, stage.relpath)
             self.assertFalse(stage.changed())
 
diff --git a/tests/unit/test_stage.py b/tests/unit/test_stage.py
index 354fb60078..ce23fad52f 100644
--- a/tests/unit/test_stage.py
+++ b/tests/unit/test_stage.py
@@ -111,5 +111,6 @@ def test_stage_run_ignore_sigint(dvc_repo, mocker):
 def test_always_changed(dvc_repo):
     stage = Stage(dvc_repo, "path", always_changed=True)
     stage.save()
-    assert stage.changed()
-    assert stage.status()["path"] == ["always changed"]
+    with dvc_repo.lock:
+        assert stage.changed()
+        assert stage.status()["path"] == ["always changed"]