Commit 88089d2d authored by Benjamin Schubert's avatar Benjamin Schubert Committed by richardmaw-codethink

Extract atomic move function to utils.py

Moving atomically a file/directory can be tricky since different
errors might be raised for the same underlying problem.

Having a utility function to reduce this discrepancies will help
in ensuring we have correct behavior
parent a6defc0b
......@@ -86,7 +86,6 @@ This plugin also utilises the following configurable core plugin warnings:
"""
import os
import errno
import re
import shutil
from collections.abc import Mapping
......@@ -97,6 +96,7 @@ from configparser import RawConfigParser
from buildstream import Source, SourceError, Consistency, SourceFetcher
from buildstream import utils
from buildstream.plugin import CoreWarnings
from buildstream.utils import move_atomic, DirectoryExistsError
GIT_MODULES = '.gitmodules'
......@@ -141,24 +141,16 @@ class GitMirror(SourceFetcher):
fail="Failed to clone git repository {}".format(url),
fail_temporarily=True)
self._atomic_move_mirror(tmpdir, url)
def _atomic_move_mirror(self, tmpdir, url):
# Attempt atomic rename into destination, this will fail if
# another process beat us to the punch
try:
os.rename(tmpdir, self.mirror)
except OSError as e:
# When renaming and the destination repo already exists, os.rename()
# will fail with either ENOTEMPTY or EEXIST, depending on the underlying
# implementation.
# An empty directory would always be replaced.
if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
self.source.status("{}: Discarding duplicate clone of {}"
.format(self.source, url))
else:
raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
.format(self.source, url, tmpdir, self.mirror, e)) from e
try:
move_atomic(tmpdir, self.mirror)
except DirectoryExistsError:
# Another process was quicker to download this repository.
# Let's discard our own
self.source.status("{}: Discarding duplicate clone of {}"
.format(self.source, url))
except OSError as e:
raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
.format(self.source, url, tmpdir, self.mirror, e)) from e
def _fetch(self, alias_override=None):
url = self.source.translate_url(self.url,
......
......@@ -72,6 +72,11 @@ class ProgramNotFoundError(BstError):
super().__init__(message, domain=ErrorDomain.PROG_NOT_FOUND, reason=reason)
class DirectoryExistsError(OSError):
"""Raised when a `os.rename` is attempted but the destination is an existing directory.
"""
class FileListResult():
"""An object which stores the result of one of the operations
which run on a list of files.
......@@ -500,6 +505,38 @@ def get_bst_version():
.format(__version__))
def move_atomic(source, destination, ensure_parents=True):
"""Move the source to the destination using atomic primitives.
This uses `os.rename` to move a file or directory to a new destination.
It wraps some `OSError` thrown errors to ensure their handling is correct.
The main reason for this to exist is that rename can throw different errors
for the same symptom (https://www.unix.com/man-page/POSIX/3posix/rename/).
We are especially interested here in the case when the destination already
exists. In this case, either EEXIST or ENOTEMPTY are thrown.
In order to ensure consistent handling of these exceptions, this function
should be used instead of `os.rename`
Args:
source (str or Path): source to rename
destination (str or Path): destination to which to move the source
ensure_parents (bool): Whether or not to create the parent's directories
of the destination (default: True)
"""
if ensure_parents:
os.makedirs(os.path.dirname(str(destination)), exist_ok=True)
try:
os.rename(str(source), str(destination))
except OSError as exc:
if exc.errno in (errno.EEXIST, errno.ENOTEMPTY):
raise DirectoryExistsError(*exc.args) from exc
raise
@contextmanager
def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
errors=None, newline=None, closefd=True, opener=None, tempdir=None):
......
......@@ -21,14 +21,11 @@
#
import os
from unittest import mock
import pytest
import py.path
from buildstream._exceptions import ErrorDomain
from buildstream import _yaml, SourceError
from buildstream import _yaml
from buildstream.plugin import CoreWarnings
from buildstream.plugins.sources.git import GitMirror
from tests.testutils import cli, create_repo
from tests.testutils.site import HAVE_GIT
......@@ -39,64 +36,6 @@ DATA_DIR = os.path.join(
)
@pytest.mark.parametrize(
["type_", "setup"],
[
("inexistent-dir", lambda path: None),
("empty-dir", lambda path: path.mkdir()),
("non-empty-dir", lambda path: path.join("bad").write("hi", ensure=True)),
("file", lambda path: path.write("no")),
],
)
def test_git_mirror_atomic_move(tmpdir, type_, setup):
# Create required elements
class DummySource:
def get_mirror_directory(self):
return str(tmpdir.join("source").mkdir())
status = mock.MagicMock()
url = "file://{}/url".format(tmpdir)
# Create fake download directory
download_dir = tmpdir.join("download_dir")
download_dir.join("oracle").write("hello", ensure=True)
# Initiate mirror
mirror = GitMirror(DummySource(), None, url, None)
# Setup destination directory
setup(py.path.local(mirror.mirror))
# Copy directory content
if type_ == "file":
with pytest.raises(SourceError):
mirror._atomic_move_mirror(str(download_dir), mirror.url)
else:
mirror._atomic_move_mirror(str(download_dir), mirror.url)
# Validate
if type_ == "non-empty-dir":
# With a non empty directory, we should not have overriden
# and notified a status
assert DummySource.status.called
expected_oracle = os.path.join(mirror.mirror, "bad")
expected_content = "hi"
elif type_ == "file":
expected_oracle = mirror.mirror
expected_content = "no"
else:
# Otherwise we should have a new directory and the data
expected_oracle = os.path.join(mirror.mirror, "oracle")
expected_content = "hello"
assert os.path.exists(expected_oracle)
with open(expected_oracle) as fp:
assert fp.read() == expected_content
@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
def test_fetch_bad_ref(cli, tmpdir, datafiles):
......
import pytest
from buildstream.utils import move_atomic, DirectoryExistsError
@pytest.fixture
def src(tmp_path):
src = tmp_path.joinpath("src")
src.mkdir()
with src.joinpath("test").open("w") as fp:
fp.write("test")
return src
def test_move_to_empty_dir(src, tmp_path):
dst = tmp_path.joinpath("dst")
move_atomic(src, dst)
assert dst.joinpath("test").exists()
def test_move_to_empty_dir_create_parents(src, tmp_path):
dst = tmp_path.joinpath("nested/dst")
move_atomic(src, dst)
assert dst.joinpath("test").exists()
def test_move_to_empty_dir_no_create_parents(src, tmp_path):
dst = tmp_path.joinpath("nested/dst")
with pytest.raises(FileNotFoundError):
move_atomic(src, dst, ensure_parents=False)
def test_move_non_existing_dir(tmp_path):
dst = tmp_path.joinpath("dst")
src = tmp_path.joinpath("src")
with pytest.raises(FileNotFoundError):
move_atomic(src, dst)
def test_move_to_existing_empty_dir(src, tmp_path):
dst = tmp_path.joinpath("dst")
dst.mkdir()
move_atomic(src, dst)
assert dst.joinpath("test").exists()
def test_move_to_existing_file(src, tmp_path):
dst = tmp_path.joinpath("dst")
with dst.open("w") as fp:
fp.write("error")
with pytest.raises(NotADirectoryError):
move_atomic(src, dst)
def test_move_file_to_existing_file(tmp_path):
dst = tmp_path.joinpath("dst")
src = tmp_path.joinpath("src")
with src.open("w") as fp:
fp.write("src")
with dst.open("w") as fp:
fp.write("dst")
move_atomic(src, dst)
with dst.open() as fp:
assert fp.read() == "src"
def test_move_to_existing_non_empty_dir(src, tmp_path):
dst = tmp_path.joinpath("dst")
dst.mkdir()
with dst.joinpath("existing").open("w") as fp:
fp.write("already there")
with pytest.raises(DirectoryExistsError):
move_atomic(src, dst)
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment