From 3ab4eb3f89a20bf2018d4af2959fc217c276d984 Mon Sep 17 00:00:00 2001 From: Michael Gummelt Date: Thu, 4 Jun 2015 15:05:28 -0700 Subject: [PATCH] gracefully handle file open errors --- cli/dcoscli/config/main.py | 2 +- cli/dcoscli/marathon/main.py | 4 +-- cli/dcoscli/package/main.py | 7 ++-- cli/tests/integrations/test_package.py | 2 +- dcos/auth.py | 2 +- dcos/config.py | 5 +-- dcos/package.py | 22 ++++++------ dcos/subcommand.py | 8 ++--- dcos/util.py | 50 ++++++++++++++++++++++---- tests/test_util.py | 12 +++++++ 10 files changed, 81 insertions(+), 33 deletions(-) diff --git a/cli/dcoscli/config/main.py b/cli/dcoscli/config/main.py index bbc6cde..9778371 100644 --- a/cli/dcoscli/config/main.py +++ b/cli/dcoscli/config/main.py @@ -360,7 +360,7 @@ def _save_config_file(config_path, toml_config): """ serial = toml.dumps(toml_config._dictionary) - with open(config_path, 'w') as config_file: + with util.open_file(config_path, 'w') as config_file: config_file.write(serial) diff --git a/cli/dcoscli/marathon/main.py b/cli/dcoscli/marathon/main.py index 177ba31..a5464a5 100644 --- a/cli/dcoscli/marathon/main.py +++ b/cli/dcoscli/marathon/main.py @@ -289,8 +289,8 @@ def _get_resource(resource): :rtype: dict """ if resource is not None: - with open(resource) as fd: - return util.load_json(fd) + with util.open_file(resource) as resource_file: + return util.load_json(resource_file) # Check that stdin is not tty if sys.stdin.isatty(): diff --git a/cli/dcoscli/package/main.py b/cli/dcoscli/package/main.py index c4855cd..6dd58e9 100644 --- a/cli/dcoscli/package/main.py +++ b/cli/dcoscli/package/main.py @@ -246,11 +246,8 @@ def _user_options(path): if path is None: return {} else: - try: - with open(path) as options_file: - return util.load_json(options_file) - except IOError: - raise DCOSException('No such file: {}'.format(path)) + with util.open_file(path) as options_file: + return util.load_json(options_file) def _confirm(prompt, yes): diff --git a/cli/tests/integrations/test_package.py b/cli/tests/integrations/test_package.py index 77a78cf..0b52921 100644 --- a/cli/tests/integrations/test_package.py +++ b/cli/tests/integrations/test_package.py @@ -385,7 +385,7 @@ def test_install_missing_options_file(): returncode=1, stdout=b'We recommend a minimum of one node with at least 1 CPU and ' b'2GB of RAM available for the Chronos Service.\n', - stderr=b"No such file: asdf.json\n") + stderr=b"Error opening file [asdf.json]: No such file or directory\n") def test_package_metadata(): diff --git a/dcos/auth.py b/dcos/auth.py index 6a29c41..905dcba 100644 --- a/dcos/auth.py +++ b/dcos/auth.py @@ -136,7 +136,7 @@ def _save_auth_keys(key_dict): toml_config[name] = python_value serial = toml.dumps(toml_config._dictionary) - with open(config_path, 'w') as config_file: + with util.open_file(config_path, 'w') as config_file: config_file.write(serial) return None diff --git a/dcos/config.py b/dcos/config.py index aa2a0d9..3d0f72f 100644 --- a/dcos/config.py +++ b/dcos/config.py @@ -1,6 +1,7 @@ import collections import toml +from dcos import util def mutable_load_from_path(path): @@ -12,7 +13,7 @@ def mutable_load_from_path(path): :rtype: MutableToml """ - with open(path) as config_file: + with util.open_file(path) as config_file: return MutableToml(toml.loads(config_file.read())) @@ -25,7 +26,7 @@ def load_from_path(path): :rtype: Toml """ - with open(path) as config_file: + with util.open_file(path) as config_file: return Toml(toml.loads(config_file.read())) diff --git a/dcos/package.py b/dcos/package.py index b0ac13b..e74568f 100644 --- a/dcos/package.py +++ b/dcos/package.py @@ -630,18 +630,22 @@ def _acquire_file_lock(lock_file_path): :param lock_file_path: Path to the lock file :type lock_file_path: str - :returns: Lock file descriptor + :returns: Lock file :rtype: File """ - lock_fd = open(lock_file_path, 'w') + try: + lock_file = open(lock_file_path, 'w') + except IOError as e: + raise util.io_exception(lock_file_path, e.errno) + acquire_mode = portalocker.LOCK_EX | portalocker.LOCK_NB try: - portalocker.lock(lock_fd, acquire_mode) - return lock_fd + portalocker.lock(lock_file, acquire_mode) + return lock_file except portalocker.LockException: - lock_fd.close() + lock_file.close() raise DCOSException('Unable to acquire the package cache lock') @@ -1036,11 +1040,9 @@ class Registry(): raise DCOSException('Path [{}] is not a file'.format(index_path)) try: - with open(index_path) as fd: + with util.open_file(index_path) as fd: version_json = json.load(fd) return version_json.get('version') - except IOError: - raise DCOSException('Unable to open file [{}]'.format(index_path)) except ValueError: raise DCOSException('Unable to parse [{}]'.format(index_path)) @@ -1061,10 +1063,8 @@ class Registry(): raise DCOSException('Path [{}] is not a file'.format(index_path)) try: - with open(index_path) as fd: + with util.open_file(index_path) as fd: return json.load(fd) - except IOError: - raise DCOSException('Unable to open file [{}]'.format(index_path)) except ValueError: raise DCOSException('Unable to parse [{}]'.format(index_path)) diff --git a/dcos/subcommand.py b/dcos/subcommand.py index 0527f3c..133702c 100644 --- a/dcos/subcommand.py +++ b/dcos/subcommand.py @@ -186,7 +186,7 @@ def _write_package_json(pkg, version): package_json = pkg.package_json(version) - with open(package_path, 'w') as package_file: + with util.open_file(package_path, 'w') as package_file: json.dump(package_json, package_file) @@ -204,7 +204,7 @@ def _write_package_version(pkg, version): version_path = os.path.join(pkg_dir, 'version') - with open(version_path, 'w') as version_file: + with util.open_file(version_path, 'w') as version_file: version_file.write(version) @@ -220,7 +220,7 @@ def _write_package_source(pkg): source_path = os.path.join(pkg_dir, 'source') - with open(source_path, 'w') as source_file: + with util.open_file(source_path, 'w') as source_file: source_file.write(pkg.registry.source.url) @@ -447,5 +447,5 @@ class InstalledSubcommand(object): """ package_json_path = os.path.join(self._dir(), 'package.json') - with open(package_json_path) as package_json_file: + with util.open_file(package_json_path) as package_json_file: return util.load_json(package_json_file) diff --git a/dcos/util.py b/dcos/util.py index 91441f5..80e3cd5 100644 --- a/dcos/util.py +++ b/dcos/util.py @@ -15,7 +15,7 @@ import jsonschema import prettytable import pystache import six -from dcos import config, constants +from dcos import constants from dcos.errors import DCOSException @@ -97,11 +97,8 @@ def read_file(path): if not os.path.isfile(path): raise DCOSException('Path [{}] is not a file'.format(path)) - try: - with open(path) as fd: - return fd.read() - except IOError: - raise DCOSException('Unable to open file [{}]'.format(path)) + with open_file(path) as file_: + return file_.read() def get_config(): @@ -110,6 +107,9 @@ def get_config(): :rtype: Toml """ + # avoid circular import + from dcos import config + return config.load_from_path( os.environ[constants.DCOS_CONFIG_ENV]) @@ -509,4 +509,42 @@ def table(fields, objs, sortby=None): return tb +@contextlib.contextmanager +def open_file(path, *args): + """Context manager that opens a file, and raises a DCOSException if + it fails. + + :param path: file path + :type path: str + :param *args: other arguments to pass to `open` + :type *args: [str] + :returns: a context manager + :rtype: context manager + """ + + try: + file_ = open(path, *args) + yield file_ + except IOError as e: + raise io_exception(path, e.errno) + + file_.close() + + +def io_exception(path, errno): + """Returns a DCOSException for when there is an error opening the + file at `path` + + :param path: file path + :type path: str + :param errno: IO error number + :type errno: int + :returns: DCOSException + :rtype: DCOSException + """ + + return DCOSException('Error opening file [{}]: {}'.format( + path, os.strerror(errno))) + + logger = get_logger(__name__) diff --git a/tests/test_util.py b/tests/test_util.py index 90ec82c..cc0336d 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -1,4 +1,7 @@ from dcos import util +from dcos.errors import DCOSException + +import pytest def test_render_mustache_json(): @@ -16,3 +19,12 @@ def test_render_mustache_json(): assert result.get('x') == xs assert result.get('y') == ys assert result.get('z') == z + + +def test_open_file(): + path = 'nonexistant_file_name.txt' + with pytest.raises(DCOSException) as excinfo: + with util.open_file(path): + pass + assert 'Error opening file [{}]: No such file or directory'.format(path) \ + in str(excinfo.value)