[tor-commits] [stem/master] Add timeout argument to stem.util.system.call()

atagar at torproject.org atagar at torproject.org
Sun Feb 12 05:52:41 UTC 2017


commit f3785a139eaecf4ff896315e92b89588ae3e2c9c
Author: Damian Johnson <atagar at 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
 





More information about the tor-commits mailing list