967782 - [mozprocess] kill() should actually wait for the managed process to be stopped

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

William, what do you think?

Comment 2

[William Lachance (:wlach)]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8371699 [details] [diff] [review]
Patch v1

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

Thanks for the new tests.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://github.com/mozilla/mozbase/commit/7a06252e404dd02f34abcf9a46d2753235683a7b