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?
William, what do you think?
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.
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.
Created attachment 8371482 [details] testcase 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
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
Created attachment 8371699 [details] [diff] [review] Patch v1 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.
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+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.