Removing the implicit delete from Terminate and Kill.

testing/legion/rpc_methods.py

Index: testing/legion/rpc_methods.py
diff --git a/testing/legion/rpc_methods.py b/testing/legion/rpc_methods.py
index 7f17e2387e38b2e3c1bdd95e86fd031c74486481..1a59f288afde5fb03c1f8fb3d0ebf4fa6de5bae4 100644
--- a/testing/legion/rpc_methods.py
+++ b/testing/legion/rpc_methods.py
@@ -41,6 +41,10 @@ class RPCMethods(object):
     logging.info('Echoing %s', message)
     return 'echo %s' % str(message)
 
+  def AbsPath(self, path):
+    """Returns the absolute path."""
+    return os.path.abspath(path)
+
   def Quit(self):
     """Call _server.shutdown in another thread.
 
@@ -58,6 +62,10 @@ class Subprocess(object):
   This non-blocking subprocess allows the caller to continue operating while
   also able to interact with this subprocess based on a key returned to
   the caller at the time of creation.
+
+  Creation args are set via Set* methods called after calling Process but
+  before calling Start. This is due to a limitation of the XML-RPC
+  implementation not supporting keyword arguments.
   """
 
   _processes = {}
@@ -65,23 +73,38 @@ class Subprocess(object):
   _creation_lock = threading.Lock()
 
   def __init__(self, cmd):
-    self.proc = subprocess42.Popen(cmd, stdout=subprocess42.PIPE,
-                                   stderr=subprocess42.PIPE)
     self.stdout = ''
     self.stderr = ''
+    self.cmd = cmd
+    self.proc = None
+    self.cwd = None
+    self.verbose = False
+    self.detached = False
     self.data_lock = threading.Lock()
-    threading.Thread(target=self._run).start()
 
-  def _run(self):
+  def __str__(self):
+    return '%r, cwd=%r, verbose=%r, detached=%r' % (
+        self.cmd, self.cwd, self.verbose, self.detached)
+
+  def _reader(self):
     for pipe, data in self.proc.yield_any():
       with self.data_lock:
         if pipe == 'stdout':
           self.stdout += data
+          if self.verbose:
+            sys.stdout.write(data)
         else:
           self.stderr += data
+          if self.verbose:
+            sys.stderr.write(data)
+
+  @classmethod
+  def KillAll(cls):
+    for key in cls._processes:
+      cls.Kill(key)
 
   @classmethod
-  def Popen(cls, cmd):
+  def Process(cls, cmd):
     with cls._creation_lock:
       key = 'Process%d' % cls._process_next_id
       cls._process_next_id += 1
@@ -90,18 +113,50 @@ class Subprocess(object):
     cls._processes[key] = process
     return key
 
+  def _Start(self):
+    logging.info('Starting process %s', self)
+    self.proc = subprocess42.Popen(self.cmd, stdout=subprocess42.PIPE,
+                                   stderr=subprocess42.PIPE,
+                                   detached=self.detached, cwd=self.cwd)
+    threading.Thread(target=self._reader).start()
+
+  @classmethod
+  def Start(cls, key):
+    cls._processes[key]._Start()
+
+  @classmethod
+  def SetCwd(cls, key, cwd):

[M-A Ruel, 2015/03/09 19:50:35]

Why not set it at Process() call instead?

[Mike Meade, 2015/03/09 20:24:36]

I am expecting there to be more ctor args, and since XML-RPC doesn't support
keyword args the entire set of arguments needs to be passed if you would
normally specify it by keyword (Popen(cmd, cwd='foo').

For example currently we have cmd, cwd, detached. If we had a single method that
takes (cmd, cwd=None, detached=False) and the user wanted to make it detached
they would have to know the full signature and pass ([cmd], None, True).

This doesn't seem to scale well if I am right about the ctor args getting more
complex.

+    """Sets the process's cwd."""
+    logging.debug('Setting %s cwd to %s', key, cwd)
+    cls._processes[key].cwd = cwd
+
+  @classmethod
+  def SetDetached(cls, key):
+    """Creates a detached process."""
+    logging.debug('Setting %s to run detached', key)
+    cls._processes[key].detached = True
+
+  @classmethod
+  def SetVerbose(cls, key):
+    """Sets the stdout and stderr to be emitted locally."""

[M-A Ruel, 2015/03/09 19:50:35]

I'm not sure why it is useful that this is exposed? When would you switch this?

[Mike Meade, 2015/03/09 20:24:36]

Its been useful to have the process print out the stdout/stderr on the task
machine when developing/running the chromoting integration tests on a task
machine. I was simply able to run the command and pull up the task web page and
see the output in real time. When we get 2 and 3 machines it will help to keep
the outputs separate while running.

However when running multiple processes on a single machine (test + web server)
its useful to suppress one or more outputs to make reading a single process's
output easier.

There just seem to be too many variations and too easy to get confused to not
support the ability to enable and disable echo'ing of stdout/stderr on the task
machine. I would rather give that power to the developer.

+    logging.debug('Setting %s to be verbose', key)
+    cls._processes[key].verbose = True
+
   @classmethod
   def Terminate(cls, key):
-    logging.debug('Terminating and deleting process %s', key)
-    return cls._processes.pop(key).proc.terminate()
+    logging.debug('Terminating process %s', key)
+    cls._processes[key].proc.terminate()
 
   @classmethod
   def Kill(cls, key):
-    logging.debug('Killing and deleting process %s', key)
-    return cls._processes.pop(key).proc.kill()
+    logging.debug('Killing process %s', key)
+    cls._processes[key].proc.kill()
 
   @classmethod
   def Delete(cls, key):
+    if cls.GetReturncode(key) is None:
+      logging.warning('Killing %s before deleting it', key)
+      cls.Kill(key)
     logging.debug('Deleting process %s', key)
     cls._processes.pop(key)
 
@@ -139,6 +194,14 @@ class Subprocess(object):
     return stderr
 
   @classmethod
+  def ReadOutput(cls, key):
+    """Returns the (stdout, stderr) since the last Read* call.
+
+    See ReadStdout for additional details.
+    """
+    return cls.ReadStdout(key), cls.ReadStderr(key)
+
+  @classmethod
   def Wait(cls, key):
     return cls._processes[key].proc.wait()