Closed
Bug 967782
Opened 10 years ago
Closed 10 years ago
[mozprocess] kill() should actually wait for the managed process to be stopped
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(1 file, 1 obsolete file)
7.40 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
As what I have seen while working on bug 965326, is that poll() still returns None, even after the process should have been killed: p = processhandler.ProcessHandler([self.python, self.proclaunch, "process_normal_finish.ini"], cwd=here) p.run() p.kill() returncode = p.poll() Here the returncode is None. Given that the process should be stopped when kill() returns, the returncode should not be None. Maybe kill() doesn't wait for the outputThread to be finsihed?
Comment 2•10 years ago
|
||
I recall that :ahal ran into something like this a while back; I don't think poll() always returns the value you expect. I think the solution here is for mozprocess's poll() to return self.returncode if this value is set.
Flags: needinfo?(wlachance)
Assignee | ||
Comment 3•10 years ago
|
||
So I checked that again. Right after calling kill() the returncode is set to -9, which is fine. But at this moment the outThread is still active and hasn't finished processing the data yet. So if we wouldn't wait for its ending, we might have dataloss for data coming through stdout. That's something I haven't tested yet, but could imagine it would be important for e.g. log data. Also adding Andrew for needinfo, given that he might also know more.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 4•10 years ago
|
||
Simple testcase. So kill() returns -9 but a following poll() returns None again. kill() should not assume that the process already got killed. When you execute this test you will get the following output: $ python process.py Expect returncode of -9 but got None I don't think any feedback is necessary from Andrew. That's clearly a bug. The only question is if kill() should wait for the process to be shutdown or if a following wait() call should be done by the consumer.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 5•10 years ago
|
||
As discussed with jgriffin on IRC we do not see why kill shouldn't call wait() and returning when the managed process is actually gone. Patch is upcoming in a bit.
Summary: [mozprocess] Calling poll() after kill() should not return a returncode of None → [mozprocess] kill() should actually wait for the managed process to be stopped
Assignee | ||
Comment 6•10 years ago
|
||
Final patch which reworks how Process.kill() works and does not blindly set the returncode based on the signal parameter. Also 6 more tests have been added.
Attachment #8371482 -
Attachment is obsolete: true
Attachment #8371699 -
Flags: review?(jgriffin)
Comment 7•10 years ago
|
||
Comment on attachment 8371699 [details] [diff] [review] Patch v1 Review of attachment 8371699 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the new tests.
Attachment #8371699 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://github.com/mozilla/mozbase/commit/7a06252e404dd02f34abcf9a46d2753235683a7b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•