mozprocess.ProcessHandlerMixin does not handle missing property "proc" on unstarted processes

RESOLVED FIXED in Firefox 39

Status

Testing
Mozbase
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ato, Assigned: parkouss)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 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.
(Reporter)

Updated

3 years ago
Summary: mozprocess.ProcessHandlerMixin does not handle missing property "proc" on unprocesses processes → mozprocess.ProcessHandlerMixin does not handle missing property "proc" on unstarted processes
(Reporter)

Comment 1

3 years ago
Created attachment 8510487 [details]
proctest.py
(Reporter)

Comment 2

3 years ago
Created attachment 8511131 [details] [diff] [review]
Handle calls to kill, pid, and poll for unstarted mozprocess'es. r=ahal

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

Updated

3 years ago
Assignee: nobody → ato
(Assignee)

Comment 3

3 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. :)
Flags: needinfo?(ato)
(Reporter)

Comment 4

3 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.
Flags: needinfo?(ato)
(Reporter)

Comment 5

3 years ago
Unassigning myself to indicate I'm not actively working on this.
Assignee: ato → nobody
(Assignee)

Comment 6

3 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 ?
Flags: needinfo?(ahalberstadt)
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.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 8

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

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.
Attachment #8578473 - Flags: review?(ahalberstadt)
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!
Attachment #8578473 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 11

3 years ago
Ok try is green.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d976fc6f582
Assignee: nobody → j.parkouss
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d976fc6f582
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.