Closed
Bug 965326
Opened 11 years ago
Closed 11 years ago
[mozprocess] Add poll() method to ProcessHandler
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Assignee: nobody → jot
Assignee | ||
Comment 1•11 years ago
|
||
Having a patch for this issue is somewhat urgent to get bug 967595 fixed. Sorry for stealing it from you, Jarek.
Assignee: jot → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
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.
Attachment #8370258 -
Flags: review?(jmaher)
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 8370258 [details] [diff] [review]
Patch v1
The patch has problems as detected while writing the tests.
Attachment #8370258 -
Flags: review?(jmaher)
Assignee | ||
Comment 4•11 years ago
|
||
Patch with tests and a slightly update for a failure I discovered via my new tests.
Attachment #8370258 -
Attachment is obsolete: true
Attachment #8370290 -
Flags: review?(jmaher)
Comment 5•11 years ago
|
||
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.
Attachment #8370290 -
Flags: review?(jmaher)
Attachment #8370290 -
Flags: review+
Attachment #8370290 -
Flags: feedback?(wlachance)
Comment 6•11 years ago
|
||
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?
Attachment #8370290 -
Flags: feedback?(wlachance) → feedback-
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
Updated patch as instructed, and fixed one more test failure because one test didn't kill the proclaunch.py process.
Attachment #8370290 -
Attachment is obsolete: true
Attachment #8370655 -
Flags: review?(wlachance)
Assignee | ||
Comment 10•11 years ago
|
||
Not sure why but the last rebase stripped of some code. Here an updated version.
Attachment #8370655 -
Attachment is obsolete: true
Attachment #8370655 -
Flags: review?(wlachance)
Attachment #8370678 -
Flags: review?(wlachance)
Assignee | ||
Comment 11•11 years ago
|
||
Just tested and all tests are passing across all platforms.
Assignee | ||
Comment 12•11 years ago
|
||
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.
Attachment #8370678 -
Flags: review?(wlachance) → review?(ted)
Comment 13•11 years ago
|
||
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?
Attachment #8370678 -
Flags: review?(ted) → review+
Assignee | ||
Comment 14•11 years ago
|
||
(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?
Flags: needinfo?(ted)
Comment 15•11 years ago
|
||
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
Flags: needinfo?(ted)
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Assignee | ||
Comment 17•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 18•11 years ago
|
||
I think I spotted a bug. Should this be 'self.proc.returncode' instead of 'self.returncode'?
Status: RESOLVED → REOPENED
Flags: needinfo?(hskupin)
Resolution: FIXED → ---
Comment 19•11 years ago
|
||
And actually pasting the line of code this time...
https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L741
Assignee | ||
Comment 20•11 years ago
|
||
(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.
Flags: needinfo?(hskupin) → needinfo?(wlachance)
Comment 21•11 years ago
|
||
I'm pretty sure Will was just saying in general and didn't mean to take his code literally. Here's the fix.
Attachment #8376283 -
Flags: review?(wlachance)
Flags: needinfo?(wlachance)
Comment 22•11 years ago
|
||
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.
Attachment #8376283 -
Flags: review?(wlachance) → review+
Comment 23•11 years ago
|
||
(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.
Comment 24•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 25•11 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•