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

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

7.40 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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?
(Assignee)

Comment 1

5 years ago
William, what do you think?
Flags: needinfo?(wlachance)
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

5 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)

Updated

5 years ago
Blocks: 967595
(Assignee)

Comment 4

5 years ago
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
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 5

5 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

5 years ago
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.
Attachment #8371482 - Attachment is obsolete: true
Attachment #8371699 - Flags: review?(jgriffin)
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

5 years ago
https://github.com/mozilla/mozbase/commit/7a06252e404dd02f34abcf9a46d2753235683a7b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 969276
You need to log in before you can comment on or make changes to this bug.