227 bytes, text/x-python-script
5.04 KB, patch
|Details | Diff | Splinter Review|
5.24 KB, patch
|Details | Diff | Splinter Review|
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.
Summary: mozprocess.ProcessHandlerMixin does not handle missing property "proc" on unprocesses processes → mozprocess.ProcessHandlerMixin does not handle missing property "proc" on unstarted processes
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.
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. :)
(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.
Unassigning myself to indicate I'm not actively working on this.
Assignee: ato → nobody
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 ?
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.
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+
Ok try is green.
Assignee: nobody → j.parkouss
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.