Revert #1736: Popen inheriting from subprocess (#1744)

5 files changed, 69 insertions, 112 deletions

diff --git a/HISTORY.rst b/HISTORY.rst
index c375934c..6f8087db 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -7,9 +7,7 @@ XXXX-XX-XX
 
 **Enhancements**
 
-- 1729_: parallel tests on UNIX (make test-parallel).
-- 1736_: psutil.Popen now inherits from subprocess.Popen instead of
-  psutil.Process. Also, wait(timeout=...) parameter is backported to Python 2.7.
+- 1729_: parallel tests on UNIX (make test-parallel). They're twice as fast!
 - 1741_: "make build/install" is now run in parallel and it's about 15% faster
   on UNIX.
 
diff --git a/docs/index.rst b/docs/index.rst
index 08a69555..133e69fe 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -1959,24 +1959,18 @@ Process class
     >>> p.terminate()
     >>> p.wait()
 
-Popen class
------------
-
 .. class:: Popen(*args, **kwargs)
 
-  A more convenient interface to stdlib `subprocess.Popen`_.
-  It starts a sub process and you deal with it exactly as when using
-  `subprocess.Popen`_, but in addition it also provides all the methods of
-  :class:`psutil.Process` class as a unified interface.
-
-  .. note::
-
-    Unlike `subprocess.Popen`_ this class preemptively checks whether PID has
-    been reused on
-    :meth:`send_signal() <psutil.Process.send_signal()>`,
-    :meth:`terminate() <psutil.Process.terminate()>` and
-    :meth:`kill() <psutil.Process.kill()>`
-    so that you can't accidentally terminate another process, fixing `BPO-6973`_.
+  Starts a sub-process via `subprocess.Popen`_, and in addition it provides
+  all the methods of :class:`psutil.Process` in a single class.
+  For method names common to both classes such as
+  :meth:`send_signal() <psutil.Process.send_signal()>`,
+  :meth:`terminate() <psutil.Process.terminate()>`,
+  :meth:`kill() <psutil.Process.kill()>` and
+  :meth:`wait() <psutil.Process.wait()>`
+  :class:`psutil.Process` implementation takes precedence.
+  This may have some advantages, like making sure PID has not been reused,
+  fixing `BPO-6973`_.
 
   >>> import psutil
   >>> from subprocess import PIPE
@@ -1992,25 +1986,10 @@ Popen class
   0
   >>>
 
-  *timeout* parameter of `subprocess.Popen.wait`_  is backported for Python < 3.3.
-  :class:`psutil.Popen` objects are supported as context managers via the with
-  statement (added to Python 3.2). On exit, standard file descriptors are
-  closed, and the process is waited for. This is supported on all Python
-  versions.
-
-  >>> import psutil, subprocess
-  >>> with psutil.Popen(["ifconfig"], stdout=subprocess.PIPE) as proc:
-  >>>     log.write(proc.stdout.read())
-
-
-  .. versionchanged:: 4.4.0 added context manager support.
-
-  .. versionchanged:: 5.7.1 inherit from `subprocess.Popen`_ instead of
-    :class:`psutil.Process`.
-
-  .. versionchanged:: 5.7.1 backporint `subprocess.Popen.wait`_ **timeout**
-    parameter on old Python versions.
+  .. versionchanged:: 4.4.0 added context manager support
 
+  .. versionchanged:: 5.7.1 wait() invokes :meth:`wait() <psutil.Process.wait()>`
+    instead of `subprocess.Popen.wait`_.
 
 Windows services
 ================
diff --git a/psutil/__init__.py b/psutil/__init__.py
index 028ab049..acf5ee79 100644
--- a/psutil/__init__.py
+++ b/psutil/__init__.py
@@ -1289,12 +1289,12 @@ _as_dict_attrnames = set(
 # =====================================================================
 
 
-class Popen(subprocess.Popen):
+class Popen(Process):
     """A more convenient interface to stdlib subprocess.Popen class.
     It starts a sub process and deals with it exactly as when using
-    subprocess.Popen class, but in addition it also provides all the
-    methods of psutil.Process class as a unified interface:
-
+    subprocess.Popen class but in addition also provides all the
+    properties and methods of psutil.Process class as a unified
+    interface:
       >>> import psutil
       >>> from subprocess import PIPE
       >>> p = psutil.Popen(["python", "-c", "print 'hi'"], stdout=PIPE)
@@ -1310,16 +1310,12 @@ class Popen(subprocess.Popen):
       >>> p.wait(timeout=2)
       0
       >>>
-
-    In addition, it backports the following functionality:
-    * "with" statement (Python 3.2)
-    * wait(timeout=...) parameter (Python 3.3)
-
+    For method names common to both classes such as kill(), terminate()
+    and wait(), psutil.Process implementation takes precedence.
     Unlike subprocess.Popen this class pre-emptively checks whether PID
-    has been reused on send_signal(), terminate() and kill(), so that
+    has been reused on send_signal(), terminate() and kill() so that
     you don't accidentally terminate another process, fixing
     http://bugs.python.org/issue6973.
-
     For a complete documentation refer to:
     http://docs.python.org/3/library/subprocess.html
     """
@@ -1328,21 +1324,21 @@ class Popen(subprocess.Popen):
         # Explicitly avoid to raise NoSuchProcess in case the process
         # spawned by subprocess.Popen terminates too quickly, see:
         # https://github.com/giampaolo/psutil/issues/193
-        self.__psproc = None
-        subprocess.Popen.__init__(self, *args, **kwargs)
-        self.__psproc = Process(self.pid)
-        self.__psproc._init(self.pid, _ignore_nsp=True)
+        self.__subproc = subprocess.Popen(*args, **kwargs)
+        self._init(self.__subproc.pid, _ignore_nsp=True)
 
     def __dir__(self):
-        return sorted(set(dir(subprocess.Popen) + dir(Process)))
+        return sorted(set(dir(Popen) + dir(subprocess.Popen)))
 
-    # Introduced in Python 3.2.
-    if not hasattr(subprocess.Popen, '__enter__'):
+    def __enter__(self):
+        if hasattr(self.__subproc, '__enter__'):
+            self.__subproc.__enter__()
+        return self
 
-        def __enter__(self):
-            return self
-
-        def __exit__(self, *args, **kwargs):
+    def __exit__(self, *args, **kwargs):
+        if hasattr(self.__subproc, '__exit__'):
+            return self.__subproc.__exit__(*args, **kwargs)
+        else:
             if self.stdout:
                 self.stdout.close()
             if self.stderr:
@@ -1360,30 +1356,21 @@ class Popen(subprocess.Popen):
             return object.__getattribute__(self, name)
         except AttributeError:
             try:
-                return object.__getattribute__(self.__psproc, name)
+                return object.__getattribute__(self.__subproc, name)
             except AttributeError:
                 raise AttributeError("%s instance has no attribute '%s'"
                                      % (self.__class__.__name__, name))
 
-    def send_signal(self, sig):
-        return self.__psproc.send_signal(sig)
-
-    def terminate(self):
-        return self.__psproc.terminate()
-
-    def kill(self):
-        return self.__psproc.kill()
-
     def wait(self, timeout=None):
-        if sys.version_info < (3, 3):
-            # backport of timeout parameter
-            if self.returncode is not None:
-                return self.returncode
-            ret = self.__psproc.wait(timeout)
-            self.returncode = ret
-            return ret
-        else:
-            return super(Popen, self).wait(timeout)
+        if self.__subproc.returncode is not None:
+            return self.__subproc.returncode
+        # Note: using psutil's wait() on UNIX should make no difference.
+        # On Windows it does, because PID can still be alive (see
+        # _pswindows.py counterpart addressing this). Python 2.7 doesn't
+        # have timeout arg, so this acts as a backport.
+        ret = Process.wait(self, timeout)
+        self.__subproc.returncode = ret
+        return ret
 
 
 # =====================================================================
diff --git a/psutil/tests/__init__.py b/psutil/tests/__init__.py
index 8ce76304..6cdf3bc8 100644
--- a/psutil/tests/__init__.py
+++ b/psutil/tests/__init__.py
@@ -486,21 +486,12 @@ def terminate(proc_or_pid, sig=signal.SIGTERM, wait_timeout=GLOBAL_TIMEOUT):
         from psutil._psposix import wait_pid
 
     def wait(proc, timeout):
-        if sys.version_info < (3, 3) and \
-                isinstance(proc, subprocess.Popen) and \
-                not isinstance(proc, psutil.Popen):
-            # subprocess.Popen instance: emulate missing timeout arg.
-            ret = None
-            try:
-                ret = psutil.Process(proc.pid).wait(timeout)
-            except psutil.NoSuchProcess:
-                # Needed to kill zombies.
-                if POSIX:
-                    ret = wait_pid(proc.pid, timeout)
-            proc.returncode = ret
-            return ret
-        else:
-            return proc.wait(timeout)
+        try:
+            return psutil.Process(proc.pid).wait(timeout)
+        except psutil.NoSuchProcess:
+            # Needed to kill zombies.
+            if POSIX:
+                return wait_pid(proc.pid, timeout)
 
     def term_subproc(proc, timeout):
         try:
diff --git a/psutil/tests/runner.py b/psutil/tests/runner.py
index ef135c8d..a7f74964 100755
--- a/psutil/tests/runner.py
+++ b/psutil/tests/runner.py
@@ -53,11 +53,21 @@ from psutil.tests import TOX
 VERBOSITY = 1 if TOX else 2
 FAILED_TESTS_FNAME = '.failed-tests.txt'
 NWORKERS = psutil.cpu_count() or 1
+USE_COLORS = term_supports_colors() and not APPVEYOR
 
 HERE = os.path.abspath(os.path.dirname(__file__))
 loadTestsFromTestCase = unittest.defaultTestLoader.loadTestsFromTestCase
 
 
+def cprint(msg, color, bold=False, file=None):
+    if file is None:
+        file = sys.stderr if color == 'red' else sys.stdout
+    if USE_COLORS:
+        print_color(msg, color, bold=bold, file=file)
+    else:
+        print(msg, file=file)
+
+
 class TestLoader:
 
     testdir = HERE
@@ -110,24 +120,21 @@ class TestLoader:
 
 class ColouredResult(unittest.TextTestResult):
 
-    def _print_color(self, s, color, bold=False):
-        print_color(s, color, bold=bold, file=self.stream)
-
     def addSuccess(self, test):
         unittest.TestResult.addSuccess(self, test)
-        self._print_color("OK", "green")
+        cprint("OK", "green")
 
     def addError(self, test, err):
         unittest.TestResult.addError(self, test, err)
-        self._print_color("ERROR", "red", bold=True)
+        cprint("ERROR", "red", bold=True)
 
     def addFailure(self, test, err):
         unittest.TestResult.addFailure(self, test, err)
-        self._print_color("FAIL", "red")
+        cprint("FAIL", "red")
 
     def addSkip(self, test, reason):
         unittest.TestResult.addSkip(self, test, reason)
-        self._print_color("skipped: %s" % reason.strip(), "brown")
+        cprint("skipped: %s" % reason.strip(), "brown")
 
     def printErrorList(self, flavour, errors):
         flavour = hilite(flavour, "red", bold=flavour == 'ERROR')
@@ -139,11 +146,7 @@ class ColouredTextRunner(unittest.TextTestRunner):
     A coloured text runner which also prints failed tests on KeyboardInterrupt
     and save failed tests in a file so that they can be re-run.
     """
-
-    if term_supports_colors() and not APPVEYOR:
-        resultclass = ColouredResult
-    else:
-        resultclass = unittest.TextTestResult
+    resultclass = ColouredResult if USE_COLORS else unittest.TextTestResult
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -180,11 +183,11 @@ class ColouredTextRunner(unittest.TextTestRunner):
 
     def _exit(self, success):
         if success:
-            print_color("SUCCESS", "green", bold=True)
+            cprint("SUCCESS", "green", bold=True)
             safe_rmpath(FAILED_TESTS_FNAME)
             sys.exit(0)
         else:
-            print_color("FAILED", "red", bold=True)
+            cprint("FAILED", "red", bold=True)
             self._write_last_failed()
             sys.exit(1)
 
@@ -228,8 +231,8 @@ class ParallelRunner(ColouredTextRunner):
         par_suite = self._parallelize(par_suite)
 
         # run parallel
-        print_color("starting parallel tests using %s workers" % NWORKERS,
-                    "green", bold=True)
+        cprint("starting parallel tests using %s workers" % NWORKERS,
+               "green", bold=True)
         t = time.time()
         par = self._run(par_suite)
         par_elapsed = time.time() - t
@@ -239,7 +242,7 @@ class ParallelRunner(ColouredTextRunner):
         orphans = psutil.Process().children()
         gone, alive = psutil.wait_procs(orphans, timeout=1)
         if alive:
-            print_color("alive processes %s" % alive, "red")
+            cprint("alive processes %s" % alive, "red")
             reap_children()
 
         # run serial
@@ -274,8 +277,7 @@ class ParallelRunner(ColouredTextRunner):
 
 def get_runner(parallel=False):
     def warn(msg):
-        print_color(msg + " Running serial tests instead.",
-                    "red", file=sys.stderr)
+        cprint(msg + " Running serial tests instead.", "red")
     if parallel:
         if psutil.WINDOWS:
             warn("Can't run parallel tests on Windows.")