935677 - Exit status reported by `wait` is incorrect

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Mike Pennisi [:jugglinmike]]

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.

Comment 1

[Mike Pennisi [:jugglinmike]]

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

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

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.

Comment 4

[Mike Pennisi [:jugglinmike]]

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

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Andrew Halberstadt [:ahal]]

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

Comment 7

[Andrew Halberstadt [:ahal]]

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

Comment 8

[William Lachance (:wlach)]

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

Comment 9

[Andrew Halberstadt [:ahal]]

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.