As seen on bug 962495 we miss a poll() method on the ProcessHandler. To be consistent with the subprocess module we should get this method added. When this gets done please also update the code in mozrunner.

Having a patch for this issue is somewhat urgent to get bug 967595 fixed. Sorry for stealing it from you, Jarek.

Patch as needed for bug 967595. I will check which tests we can add for mozprocess here. For now I would like to see if that is a correct implementation. At least no existing test is broken.

Comment on attachment 8370258 [details] [diff] [review] Patch v1 The patch has problems as detected while writing the tests.

Patch with tests and a slightly update for a failure I discovered via my new tests.

Comment on attachment 8370290 [details] [diff] [review] Patch v1.1 Review of attachment 8370290 [details] [diff] [review]: ----------------------------------------------------------------- this patch looks good to me, I would like additional eyes on it as well.

Comment on attachment 8370290 [details] [diff] [review] Patch v1.1 Review of attachment 8370290 [details] [diff] [review]: ----------------------------------------------------------------- I'd like my latter comment clarified (why we return 0 if there is no proc member). Other than that, looks ok. ::: mozprocess/mozprocess/processhandler.py @@ +705,5 @@ > + process hasn't terminated yet. A negative value -N indicates > + the process was killed by signal N (Unix only). > + > + """ > + if self.outThread and self.outThread.isAlive(): I would add a comment like # Hasn't terminated yet, return None @@ +710,5 @@ > + return None > + elif hasattr(self, "proc"): > + return self.proc.poll() > + else: > + return 0 Why 0 and not None here? If there is no proc, how can we say there was a returnvalue?

Also see my comment: https://bugzilla.mozilla.org/show_bug.cgi?id=967782#c2 We may want to return self.returncode instead of the results of self.proc.poll() if it exists.

(In reply to William Lachance (:wlach) from comment #6) > ::: mozprocess/mozprocess/processhandler.py > @@ +705,5 @@ > > + process hasn't terminated yet. A negative value -N indicates > > + the process was killed by signal N (Unix only). > > + > > + """ > > + if self.outThread and self.outThread.isAlive(): > > I would add a comment like > > # Hasn't terminated yet, return None I took this comment from wait(). So I will update both instances. > > + return None > > + elif hasattr(self, "proc"): > > + return self.proc.poll() > > + else: > > + return 0 > > Why 0 and not None here? If there is no proc, how can we say there was a > returnvalue? None also doesn't work. It would give the consumer the impression that the process is currently running. IMO this is even worse. So as best I mimic the behavior of wait here() and return the proc.poll() return value. In the future we might want to throw a specific exception if no proc is existent yet. (In reply to William Lachance (:wlach) from comment #7) > We may want to return self.returncode instead of the results of > self.proc.poll() if it exists. Will do.

Updated patch as instructed, and fixed one more test failure because one test didn't kill the proclaunch.py process.

Not sure why but the last rebase stripped of some code. Here an updated version.

Just tested and all tests are passing across all platforms.

Comment on attachment 8370678 [details] [diff] [review] Patch v2.1 Review of attachment 8370678 [details] [diff] [review]: ----------------------------------------------------------------- Ted, I hope that you can review it today. It's somewhat important for us because it blocks our mozmill release we have to get out ASAP. Thanks.

Comment on attachment 8370678 [details] [diff] [review] Patch v2.1 Review of attachment 8370678 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I have a few minor suggestions. ::: mozprocess/mozprocess/processhandler.py @@ +777,5 @@ > > + Returns the process exit code value: > + - None if the process hasn't terminated yet > + - A negative number if the process was killed by signal N (Unix only) > + - '0' if the process ended without failures It would probably be better to just say "see poll" here (with the appropriate markup to make that a link). ::: mozprocess/tests/proctest.py @@ +72,5 @@ > self.assertTrue(returncode, "Detected an unexpected return code of: %s" % returncode) > + elif isalive: > + self.assertEqual(returncode, None, "Detected not None return code of: %s" % returncode) > + else: > + self.assertNotEqual(returncode, None, "Detected unexpected None return code of") These errors are a little confusing to read, perhaps: "Expected return code == None but found %s" "Expected return code != None" ::: mozprocess/tests/test_mozprocess_poll.py @@ +16,5 @@ > + > + p = processhandler.ProcessHandler([self.python, self.proclaunch, > + "process_normal_finish_python.ini"], > + cwd=here) > + self.assertRaises(AttributeError, p.poll) This seems like unfortunate behavior. Perhaps we should simply make poll return None if the process hasn't started yet?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13) > ::: mozprocess/tests/test_mozprocess_poll.py > @@ +16,5 @@ > > + > > + p = processhandler.ProcessHandler([self.python, self.proclaunch, > > + "process_normal_finish_python.ini"], > > + cwd=here) > > + self.assertRaises(AttributeError, p.poll) > > This seems like unfortunate behavior. Perhaps we should simply make poll > return None if the process hasn't started yet? Please my comments from above. 'None' means the process is running. So I don't think that this should be the value to be returned if the process is not running yet. The code as what I have right now is exactly the same as for wait(). It has the same behavior: https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L761 So I would suggest we really tackle this on another bug. Would that be ok for you?

Pushing that to a followup is fine. Do you really think callers need to discern between "hasn't started" and "still running" by using poll()? For comparison, subprocess.Popen's poll/returncode just returns None "if the process hasn't terminated yet": http://docs.python.org/2/library/subprocess.html#subprocess.Popen.returncode

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15) > Pushing that to a followup is fine. Do you really think callers need to > discern between "hasn't started" and "still running" by using poll()? For > comparison, subprocess.Popen's poll/returncode just returns None "if the > process hasn't terminated yet": > http://docs.python.org/2/library/subprocess.html#subprocess.Popen.returncode This is bug 968446 now.

I think I spotted a bug. Should this be 'self.proc.returncode' instead of 'self.returncode'?

And actually pasting the line of code this time... https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L741

(In reply to Andrew Halberstadt [:ahal] from comment #18) > I think I spotted a bug. Should this be 'self.proc.returncode' instead of > 'self.returncode'? I'm not sure. The comment from William said something else and I have used it: (In reply to William Lachance (:wlach) from comment #7) > We may want to return self.returncode instead of the results of > self.proc.poll() if it exists. So after checking the code in detail I do not see that we have returncode under process_handler. So William might have been wrong with this statement. As best we get clarification from him. Putting up needinfo.

I'm pretty sure Will was just saying in general and didn't mean to take his code literally. Here's the fix.

Comment on attachment 8376283 [details] [diff] [review] Fix poll to use proc.returncode Review of attachment 8376283 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I think I was just confused about the distinction between ProcessHandler and its containing class. This looks right.

(In reply to William Lachance (:wlach) from comment #22) > Comment on attachment 8376283 [details] [diff] [review] > Fix poll to use proc.returncode > > Review of attachment 8376283 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes, I think I was just confused about the distinction between > ProcessHandler and its containing class. This looks right. er, Process and its containing class.

That checkin caused a test failure in our new mozrunner tests. Turns out it caught a legitimate bug! whimboo++. I'll fix it in bug 972966.