Andreas Tolfsen ❲:ato❳ Reporter
	Description • 10 years ago

mozprocess.ProcessHandlerMixin populates the self.proc property only after the run method has been called.  This means that calling other methods which rely on self.proc will fail.  Two such methods (there are possibly more) are kill() and poll().

Calling these before the process has been started will cause AttributeError: 'ProcessHandler' object has no attribute 'proc' to be raised.  See attached, non-exhaustive TC.

I suggest we change poll() to also return None when a process hasn't been started.  This conflicts with the meaning of the current return value, but I think this should be fine.

For kill(), we can silently return when the process isn't running.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 2 • 10 years ago

This patch initially sets the proc property of ProcessHandler to None
so that methods kill, pid, and poll and more easily reason about whether
the process has been started or not.

It changes the meaning of the None return vale from poll to indicate that
the process either has not been started _or_ has terminated.  It's advised
that consumers check both poll and returncode, or the type of poll's
return value explicitly to determine that the process has stopped.

The patch also sets the returncode property on ProcessHandler when
calling poll.

	Julien Pagès (:parkouss) Assignee
	Comment 3 • 9 years ago

Hi Andreas,

What is the state of this patch now ? Is it ready for review ? Maybe this needs to be reworked now because of code changes, but it would be great to not raise these attribute errors.

If you don't have time now, I can work on this if you like. :)

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 4 • 9 years ago

(In reply to Julien Pagès from comment #3)
> What is the state of this patch now ? Is it ready for review ? Maybe this
> needs to be reworked now because of code changes, but it would be great to
> not raise these attribute errors.

Yes it would (-:

I think the status of this patch is that I did a full try run (-u all -t all) and it broke _a lot_ of code depending on the old, broken behaviour.

I haven't had time to follow up further after that, but if you'd like to try reapplying this patch and doing a try run to verify that I wasn't seeing things you're more than welcome to do so.

I don't think you should have any trouble applying the patch on top of m-c because mozprocess changes very seldom.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 5 • 9 years ago

Unassigning myself to indicate I'm not actively working on this.

	Julien Pagès (:parkouss) Assignee
	Comment 6 • 9 years ago

So the patch does not apply anymore. I would like to work on this. I'm just wondering how this should be fixed ? Maybe we can raise some explicit errors (like RuntimeError) when trying to kill/poll a process that has not been started.

If I undestand well, we are trying to mimic subprocess.Popen, but the main API difference is that subprocess.Popen starts the command as soon as the object is instanciated - and here we need to call run() for this, so it is hard to compare. I think it is a programming error to call poll/kill on a non started process (and thus raising RuntimeError makes sense) but maybe we want to be able to call these methods anyway and it could be good to not have to try/except for this (as :ato mentionned on irc).

To resume we have two possibilities I would say:

 - returns None when process was not started
 - raise explicit errors instead of missing attribute error

So :ahal, how should we resolve this bug ?

	Andrew Halberstadt [:ahal]
	Comment 7 • 9 years ago

I prefer raising a custom error as well. Like :ato mentioned in comment 4 it seems like silently returning can break things.

It's unfortunate that we didn't just mimic subprocess from the outset and run the process on instantiation, but it's too late (not worth it) to fix that now :(. I think it's easiest to just deal with these UX warts as best we can.

	Julien Pagès (:parkouss) Assignee
	Comment 8 • 9 years ago

Ok, I did the changes. There are some chances that would not work on full try, because:

- missing proc member on kill() was explicitly swallowed and the method returned None (printing a stderr statement only)
- the attribute error for missing proc member on poll() was tested in an unit test (that suggest that it is the "normal" behaviour, and some users may have used it...)

Well I can run a full try push after my day work to see if something's broken somewhere.

	Andrew Halberstadt [:ahal]
	Comment 9 • 9 years ago

Comment on attachment 8578473 [details] [diff] [review]
raise explicit exceptions when kill/poll are called on a non started process

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

Thanks, lgtm!

	Julien Pagès (:parkouss) Assignee
	Comment 10 • 9 years ago

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0121f7c0de

	Julien Pagès (:parkouss) Assignee
	Comment 11 • 9 years ago

Ok try is green.

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d976fc6f582

	Phil Ringnalda (:philor)
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0d976fc6f582