669359 - Remove redundant wait code from the new mozprocess

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jonathan Griffin (:jgriffin)]

Attachment: mozprocess patch

The new version of mozprocess has some redundant wait code.  It first waits in waitForFinish, then when that's done (at which point the process has either terminated normally or been killed), it calls Process.wait(), which waits again using the same timeout as was used in waitForFinish().  The result is that you could potentially end up waiting timeout*2 for a bad process, and there's a lot of complicated code that doesn't need to be there.

This patch fixes that, along with some edge case bugs in the windows code.  For instance, for processes which exit nearly immediately after they're launched, I was sometimes running into problems because self._process_events wasn't defined.

Comment 1

[cmtalbert]

Comment on attachment 543988 [details] [diff] [review]
mozprocess patch

Review of attachment 543988 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good r+, with the changes below.

::: mozprocess/mozprocess/processhandler.py
@@ +70,1 @@
>                      self._cleanup()

I think this should be in an else i.e.:
else:
  subprocess.Popen.__del__(self)

@@ +230,3 @@
>  
>                  # It's overkill, but we use Queue to signal between threads
>                  # because it handles errors more gracefully than event or condition.

Nit: move the comment about the Queue object to the place where the Queue object is being created.

Comment 2

[Jonathan Griffin (:jgriffin)]

Updated patch committed as https://github.com/mozautomation/mozmill/commit/59554b16ba1f6d3ed6b8e916c9654090995da736