Closed Bug 947974 Opened 6 years ago Closed 6 years ago

Mozprocess should allow user to specify signal used to kill process

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(2 files)

Currently I'm implementing a harness for b2g desktop reftests based on mozbase instead of automation.py, specifically the killAndGetStack functionality.

This bug isn't strictly necessary, I could always use os.kill or os.killpg instead of mozrunner.stop in the on_timeout handler, but then I'm basically duplicating code from mozprocess.kill(). It seems nicer to just provide a method to use a different signal.

A different incarnation of this bug was r-'ed because the problem was tackled a different way, but I think this would still be useful in the general case (especially since reftest doesn't come with a "handleTimeout" method whereas mochitest did):
https://bugzilla.mozilla.org/show_bug.cgi?id=919353#c17
I can't remember what my real objection was there, I guess there's no reason not to be able to do it this way.
This adds the sig option to mozprocess.kill() and mozrunner.stop(). It also adjusts mozrunner's wait to return -<signal> if the process got killed by another thread (this is consistent with what mozprocess' wait returns).
Attachment #8344783 - Flags: review?(wlachance)
Comment on attachment 8344783 [details] [diff] [review]
Patch 1.0 - add sig option and adjust mozrunner wait

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

Looks good to me after irc discussion
Attachment #8344783 - Flags: review?(wlachance) → review+
https://github.com/mozilla/mozbase/commit/50bcb04fe88b053b16fa97833c252be439bed15e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I made a stupid mistake:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31889008&tree=Try

I think the real fix is to grab the return code from the process handler instead of just setting it here. The process handler should be in charge or returning -<sig> if it detects the process was killed. Patch upcoming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 949600
Comment on attachment 8348123 [details] [diff] [review]
Patch 1.1 - mozprocess.kill should set returncode instead of mozrunner.wait

My last patch caused an error anytime mozrunner.stop() was called because we tried to set the returncode to -None. I realized mozrunner should just be grabbing the returncode from mozprocess.kill().

While making that change I also realized that mozprocess.kill() was always returning 0 no matter what since we set it to zero at the top, then used os.kill (which doesn't cause subprocess to update it's internal state apparently) and then only setting the returncode if it is None.
Attachment #8348123 - Flags: review? → review?(wlachance)
Comment on attachment 8348123 [details] [diff] [review]
Patch 1.1 - mozprocess.kill should set returncode instead of mozrunner.wait

Looks reasonable. Sorry about missing those gotchas in the last review...
Attachment #8348123 - Flags: review?(wlachance) → review+
No worries, it was my mistake.

https://github.com/mozilla/mozbase/commit/e4357d243c17fddfeb378a2d99b267acb530da1c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
I also forgot to check that the tests still worked.. bustage fix:
https://github.com/mozilla/mozbase/commit/e6d37f30d7ead9654453a2aaa908737aa4ad22cd
Blocks: 963572
This is needed to land b2g desktop reftests. The mozbase sync is still blocked by a few other issues, so I'm going to go ahead and cherry pick this into m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/c48b1f578d7d
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.