Exit status reported by `wait` is incorrect

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jugglinmike, Assigned: ahal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
The `wait` method of the `mozprocess.ProcessHandlerMixin` class is intended to return the exit status of the underlying process, but the value returned is off by a factor of 256.
(Reporter)

Comment 1

5 years ago
Created attachment 828200 [details] [review]
pull request on GitHub

I believe the return value of `wait` is currently untested. I'm happy to add tests for this, but it may require a fair amount of change to the testing architecture (so that config files can specify exit status codes). In that case, I'll need a little guidance!
Attachment #828200 - Flags: review?(ahalberstadt)
Comment on attachment 828200 [details] [review]
pull request on GitHub

This looks good, but it's causing a test to fail:
https://travis-ci.org/mozilla/mozbase/builds/13599806

It looks like the test only previously worked because on timeout we kill the process with SIGKILL which would fill the low byte of waitpid()[1] and thus make it non-zero.

I think we want to just take this as is and return 0 if the process was killed, though we should update the test. Ted, make sense to you?

p.s in general we just upload patches to bugzilla for mozbase, but since you already went ahead I'll just use that.
Attachment #828200 - Flags: review?(ahalberstadt)
Flags: needinfo?(ted)
Note that if we did do this, it could break some assumptions throughout our automation that returncode == 0 means that everything went ok. A perhaps better alternative would be to return the whole (signal, status) tuple, though this would also require in-tree automation.
(Reporter)

Comment 4

5 years ago
Created attachment 828240 [details] [diff] [review]
935677-status-code.patch

In comment 2, :ahal informed me that BugZilla is the preferred platform for code review in mozbase. I'm attaching the original patch file to conform.

I will leave the original GitHub pull request [1] open because it triggered a TravisCI run [2] that may be useful to my reviewers.

[1] https://github.com/mozilla/mozbase/pull/50
[2] https://travis-ci.org/mozilla/mozbase/builds/13599806
Attachment #828200 - Attachment is obsolete: true
I would think we should just persist the returncode value up from subprocess:
http://docs.python.org/2/library/subprocess.html#subprocess.Popen.returncode

I don't see a good reason for deviating here.
Flags: needinfo?(ted)
Actually I thought returning proc.returncode would have the same problem in that it would return 0 if the process was killed, but looking at the docs, that's not the case:
"A negative value -N indicates that the child was terminated by signal N (Unix only)."

So yeah, looks like we need to call waitpid, but return proc.poll()
Created attachment 831719 [details] [diff] [review]
0001-Bug-935677-Return-code-from-mozprocess-wait-is-wrong.patch

So I poked around at this and it turns out subprocess doesn't handle the return code properly when dealing with process groups (which is why I guess we were using os.waitpid in the first place).

So instead of returning proc.poll(), I just formatted waitpid's status the same as subprocess.returncode (i.e negative number indicates signal it was killed with) so that the return value will be the same whether or not we use process groups.
Assignee: mike → ahalberstadt
Attachment #828240 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #831719 - Flags: review?(wlachance)
Comment on attachment 831719 [details] [diff] [review]
0001-Bug-935677-Return-code-from-mozprocess-wait-is-wrong.patch

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

LGTM
Attachment #831719 - Flags: review?(wlachance) → review+
https://github.com/mozilla/mozbase/commit/6454ec06249d66054ace43749338934492bd2889

Mike, if you need this patch for something, note that this doesn't mean it is in m-c yet. If you need it there let me know and I can get it landed there too. Otherwise it will get synced as part of a larger mozbase update at some indeterminate point in the future.
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.