Skip to content

Commit

Permalink
rwlock: check that .dvc/lock is locked
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
efiop committed Jan 6, 2020
1 parent e2efb7b commit 1dcce1d
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 8 deletions.
4 changes: 4 additions & 0 deletions dvc/lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
13 changes: 10 additions & 3 deletions dvc/repo/destroy.py
Original file line number Diff line number Diff line change
@@ -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)
2 changes: 2 additions & 0 deletions dvc/stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/func/test_repro.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion tests/func/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion tests/func/test_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Expand Down
5 changes: 3 additions & 2 deletions tests/unit/test_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

0 comments on commit 1dcce1d

Please sign in to comment.