commit f3785a139eaecf4ff896315e92b89588ae3e2c9c Author: Damian Johnson atagar@torproject.org Date: Thu Feb 9 10:35:47 2017 -0800
Add timeout argument to stem.util.system.call()
While making our installer integ test more reliable I've had it take over a minute or even fail to finish at all several times. Allowing timeouts of our system calls is generally useful anyway so good excuse to make this addition. --- docs/change_log.rst | 1 + run_tests.py | 9 +++++++- stem/util/system.py | 40 +++++++++++++++++++++++++++++++++--- test/integ/installation.py | 51 +++++++++++++++++++++++----------------------- test/integ/util/system.py | 7 +++++++ test/unit/manual.py | 1 + 6 files changed, 80 insertions(+), 29 deletions(-)
diff --git a/docs/change_log.rst b/docs/change_log.rst index 85619b2..338150c 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -55,6 +55,7 @@ The following are only available within Stem's `git repository
* **Utilities**
+ * Added timeout argument to :func:`~stem.util.system.call` * Added :class:`~stem.util.test_tools.TimedTestRunner` and :func:`~stem.util.test_tools.test_runtimes`
.. _version_1.5: diff --git a/run_tests.py b/run_tests.py index 3db6280..501cbfa 100755 --- a/run_tests.py +++ b/run_tests.py @@ -213,9 +213,10 @@ def main():
our_version = stem.version.get_system_tor_version(args.tor_path) skipped_targets = [] + integ_setup_thread = None
if not args.specific_test or 'test.integ.installation'.startswith(args.specific_test): - test.integ.installation.setup() + integ_setup_thread = test.integ.installation.setup()
for target in args.run_targets: # check if we meet this target's tor version prerequisites @@ -247,6 +248,9 @@ def main(): # We should have joined on all threads. If not then that indicates a # leak that could both likely be a bug and disrupt further targets.
+ if integ_setup_thread: + integ_setup_thread.join() + active_threads = threading.enumerate()
if len(active_threads) > 1: @@ -267,6 +271,9 @@ def main(): except OSError: error_tracker.register_error() finally: + if integ_setup_thread: + test.integ.installation.clean() + println() integ_runner.stop() println() diff --git a/stem/util/system.py b/stem/util/system.py index c1db6d0..4951058 100644 --- a/stem/util/system.py +++ b/stem/util/system.py @@ -166,6 +166,20 @@ class CallError(OSError): return self.msg
+class CallTimeoutError(CallError): + """ + Error response when making a system call that has timed out. + + .. versionadded:: 1.6.0 + + :var float timeout: time we waited + """ + + def __init__(self, msg, command, exit_status, runtime, stdout, stderr, timeout): + super(CallTimeoutError, self).__init__(msg, command, exit_status, runtime, stdout, stderr) + self.timeout = timeout + + def is_windows(): """ Checks if we are running on Windows. @@ -1020,7 +1034,7 @@ def files_with_suffix(base_path, suffix): yield os.path.join(root, filename)
-def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): +def call(command, default = UNDEFINED, ignore_exit_status = False, timeout = None, env = None): """ call(command, default = UNDEFINED, ignore_exit_status = False)
@@ -1035,16 +1049,22 @@ def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): .. versionchanged:: 1.5.0 Added env argument.
+ .. versionchanged:: 1.6.0 + Added timeout argument. + :param str,list command: command to be issued :param object default: response if the query fails :param bool ignore_exit_status: reports failure if our command's exit status was non-zero + :param float timeout: maximum seconds to wait, blocks indefinitely if + **None** :param dict env: environment variables
:returns: **list** with the lines of output from the command
- :raises: **CallError** if this fails and no default was provided, this is an - **OSError** subclass + :raises: + * **CallError** if this fails and no default was provided + * **CallTimeoutError** if the timeout is reached without a default """
global SYSTEM_CALL_TIME @@ -1062,6 +1082,13 @@ def call(command, default = UNDEFINED, ignore_exit_status = False, env = None):
process = subprocess.Popen(command_list, stdout = subprocess.PIPE, stderr = subprocess.PIPE, shell = is_shell_command, env = env)
+ if timeout: + while process.poll() is None: + if time.time() - start_time > timeout: + raise CallTimeoutError("Process didn't finish after %0.1f seconds" % timeout, ' '.join(command_list), None, timeout, '', '', timeout) + + time.sleep(0.001) + stdout, stderr = process.communicate() stdout, stderr = stdout.strip(), stderr.strip() runtime = time.time() - start_time @@ -1085,6 +1112,13 @@ def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): return stdout.decode('utf-8', 'replace').splitlines() else: return [] + except CallTimeoutError as exc: + log.debug('System call (timeout): %s (after %0.4fs)' % (command, timeout)) + + if default != UNDEFINED: + return default + else: + raise except OSError as exc: log.debug('System call (failed): %s (error: %s)' % (command, exc))
diff --git a/test/integ/installation.py b/test/integ/installation.py index cfe771a..2048ea3 100644 --- a/test/integ/installation.py +++ b/test/integ/installation.py @@ -15,6 +15,7 @@ import test.util INSTALL_MISMATCH_MSG = "Running 'python setup.py sdist' doesn't match our git contents in the following way. The manifest in our setup.py may need to be updated...\n\n"
BASE_INSTALL_PATH = '/tmp/stem_test' +DIST_PATH = os.path.join(test.util.STEM_BASE, 'dist') SETUP_THREAD, INSTALL_FAILURE, INSTALL_PATH, SDIST_FAILURE = None, None, None, None
@@ -35,8 +36,8 @@ def setup():
try: os.chdir(test.util.STEM_BASE) - stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, BASE_INSTALL_PATH)) - stem.util.system.call('%s setup.py clean --all' % sys.executable) # tidy up the build directory + stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, BASE_INSTALL_PATH), timeout = 60) + stem.util.system.call('%s setup.py clean --all' % sys.executable, timeout = 60) # tidy up the build directory site_packages_paths = glob.glob('%s/lib*/*/site-packages' % BASE_INSTALL_PATH)
if len(site_packages_paths) != 1: @@ -46,10 +47,13 @@ def setup(): except Exception as exc: INSTALL_FAILURE = AssertionError("Unable to install with 'python setup.py install': %s" % exc)
- try: - stem.util.system.call('%s setup.py sdist' % sys.executable) - except Exception as exc: - SDIST_FAILURE = exc + if not os.path.exists(DIST_PATH): + try: + stem.util.system.call('%s setup.py sdist' % sys.executable, timeout = 60) + except Exception as exc: + SDIST_FAILURE = exc + else: + SDIST_FAILURE = AssertionError("%s already exists, maybe you manually ran 'python setup.py sdist'?" % DIST_PATH) finally: os.chdir(original_cwd)
@@ -60,6 +64,14 @@ def setup(): return SETUP_THREAD
+def clean(): + if os.path.exists(BASE_INSTALL_PATH): + shutil.rmtree(BASE_INSTALL_PATH) + + if os.path.exists(DIST_PATH): + shutil.rmtree(DIST_PATH) + + def _assert_has_all_files(path): """ Check that all the files in the stem directory are present in the @@ -100,17 +112,14 @@ class TestInstallation(unittest.TestCase): install. """
- try: + if not INSTALL_PATH: setup().join()
- if INSTALL_FAILURE: - raise INSTALL_FAILURE + if INSTALL_FAILURE: + raise INSTALL_FAILURE
- self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % INSTALL_PATH])[0]) - _assert_has_all_files(INSTALL_PATH) - finally: - if os.path.exists(BASE_INSTALL_PATH): - shutil.rmtree(BASE_INSTALL_PATH) + self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % INSTALL_PATH])[0]) + _assert_has_all_files(INSTALL_PATH)
@test.runner.only_run_once def test_sdist(self): @@ -129,20 +138,12 @@ class TestInstallation(unittest.TestCase): if SDIST_FAILURE: raise SDIST_FAILURE
- original_cwd = os.getcwd() - dist_path = os.path.join(test.util.STEM_BASE, 'dist') git_contents = [line.split()[-1] for line in stem.util.system.call('git ls-tree --full-tree -r HEAD')]
- try: - # tarball has a prefix 'stem-[verion]' directory so stipping that out - - dist_tar = tarfile.open(os.path.join(dist_path, 'stem-dry-run-%s.tar.gz' % stem.__version__)) - tar_contents = ['/'.join(info.name.split('/')[1:]) for info in dist_tar.getmembers() if info.isfile()] - finally: - if os.path.exists(dist_path): - shutil.rmtree(dist_path) + # tarball has a prefix 'stem-[verion]' directory so stipping that out
- os.chdir(original_cwd) + dist_tar = tarfile.open(os.path.join(DIST_PATH, 'stem-dry-run-%s.tar.gz' % stem.__version__)) + tar_contents = ['/'.join(info.name.split('/')[1:]) for info in dist_tar.getmembers() if info.isfile()]
issues = []
diff --git a/test/integ/util/system.py b/test/integ/util/system.py index dd301f9..b6e66a0 100644 --- a/test/integ/util/system.py +++ b/test/integ/util/system.py @@ -563,6 +563,13 @@ class TestSystem(unittest.TestCase): self.assertEqual(home_dir, stem.util.system.expand_path('~%s' % username)) self.assertEqual(os.path.join(home_dir, 'foo'), stem.util.system.expand_path('~%s/foo' % username))
+ def test_call_timeout(self): + try: + stem.util.system.call('sleep 1', timeout = 0.001) + self.fail("sleep should've timed out") + except stem.util.system.CallTimeoutError as exc: + self.assertEqual("Process didn't finish after 0.0 seconds", str(exc)) + def test_call_time_tracked(self): """ Check that time taken in the call() function is tracked by SYSTEM_CALL_TIME. diff --git a/test/unit/manual.py b/test/unit/manual.py index 8338af7..5f939c3 100644 --- a/test/unit/manual.py +++ b/test/unit/manual.py @@ -105,6 +105,7 @@ EXPECTED_CONFIG_OPTIONS['MaxAdvertisedBandwidth'] = stem.manual.ConfigOption(
CACHED_MANUAL = None
+ def _cached_manual(): global CACHED_MANUAL