mozprocess ProcessHandler.kill can hang
Categories
(Testing :: Mozbase, defect)
Tracking
(firefox90 fixed)
| Tracking | Status | |
|---|---|---|
| firefox90 | --- | fixed |
People
(Reporter: jgraham, Assigned: jgraham)
References
Details
Attachments
(8 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
The TSAN tests are causing problems in wpt; the wpt harness tries to signal the Firefox process using mozrunner::Runner.stop(), which ends up calling kill() in ProcessHandlerMixin, which (unlike the subprocess::Popen case) calls wait, which (on unix) ends up calling os.waitpid which will never return if the process doesn't quit. So this precludes a pattern like "send SIGTERM and if that fails send SIGKILL".
Looking at the code in mozprocess, it's unclear what problem the _custom_wait function (on posix) is solving that isn't solved by the modern stdlib wait; afaict they both end up calling os.waitpid to do the actual waiting, but the stdlib version has support for a timeout. The comments in mozprocess suggest it's supposed to handle process trees better, but I can't see anything in the implementation to support this; possibly the stdlib implementation improved in the last 10 years.
I suggest we allow passing a timeout into mozrunner::stop and replacing the mozprocess _custom_wait function with stdlib wait to support that.
|
Assignee | |
Comment 1•4 years ago
|
||
This is not obviously doing more than the stdlib code does these days,
and also doesn't support timeouts, so we're perhaps better just always
defering to the stdlib.
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 2•4 years ago
|
||
Unlike the stdlib Popen class, ProcessHandler.kill implictly waits for
the process to end. To avoid hangs in the case where the process
doesn't end, allow passing through a timeout to this function (on
posix only).
This is somewhat unfortunate as it means that ProcessHandler isn't
interchangable with Popen. But the only other option here appears to
be not doing the implicit wait which presumably consumers are relying on.
|
|
Assignee | |
Comment 3•4 years ago
|
||
This allows us to handle the case where the runner doesn't stop
e.g. by using a more aggressive signal.
|
|
Assignee | |
Comment 4•4 years ago
|
||
This allows the SIGTERM-then-SIGKILL logic to work as intended.
|
|
Assignee | |
Comment 5•4 years ago
|
||
This is not obviously doing more than the stdlib code does these days,
and also doesn't support timeouts, so we're perhaps better just always
defering to the stdlib.
|
|
Assignee | |
Comment 6•4 years ago
|
||
Unlike the stdlib Popen class, ProcessHandler.kill implictly waits for
the process to end. To avoid hangs in the case where the process
doesn't end, allow passing through a timeout to this function (on
posix only).
This is somewhat unfortunate as it means that ProcessHandler isn't
interchangable with Popen. But the only other option here appears to
be not doing the implicit wait which presumably consumers are relying on.
|
|
Assignee | |
Comment 7•4 years ago
|
||
This allows us to handle the case where the runner doesn't stop
e.g. by using a more aggressive signal.
|
|
Assignee | |
Comment 8•4 years ago
|
||
This allows the SIGTERM-then-SIGKILL logic to work as intended.
|
||
Comment 11•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/1e8842ace029
https://hg.mozilla.org/mozilla-central/rev/4dc5ae176466
https://hg.mozilla.org/mozilla-central/rev/f230efb7a553
https://hg.mozilla.org/mozilla-central/rev/44a9d348b719
Description
•