921199 - mozrunner.wait() should do something other than return None when its process_handler doesn't exist

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L71

Bug 915866 makes mozrunner.wait() return the return code of the process or None if the process is still running. This is in-line with how the python subprocess module works.

The problem is mozrunner.wait() also returns None if its process_handler doesn't exist (this could happen if wait is called after the process has terminated, or before the process has started). This can lead consumers to believe the call has timed out but is still running which is not the case.

I think we should do the following:
1) When the process is finished, save the return code in a self.return_code variable and return it
2)
a) if self.process_handler is not None: do the wait as normal
b) if self.process_handler is None and self.return_code is not None: return self.return_code (wait called after process terminated)
c) if self.process_handler is None and self.return_code is None: raise an exception (wait called before start)

Comment 1

[Jeff Hammel]

+1

> 1) When the process is finished, save the return code in a
> self.return_code variable and return it

I would not oppose printing/logging "Process already finished" in some sort of hypothetical verbose

Comment 2

[Andrew Halberstadt [:ahal]]

Sebastiaan has kindly volunteered to work on this for us

Comment 3

[Sebastiaan de Haan [:kyr0]]

Attachment: Attempt to apply the changes requested.

My first crack at this, I do believe however that the else statement at line 80 could be removed. Please advice.

Comment 4

[Sebastiaan de Haan [:kyr0]]

This is not right... Sorry.

Comment 5

[Sebastiaan de Haan [:kyr0]]

Attachment: Correct patch

This time with the correct patch.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 818487 [details] [diff] [review]
Correct patch

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

This was close, you had everything we needed, just not quite the right indentation level :). I know the requirements were very confusing, so not bad at all (especially given how it's hard to test)!

::: mozrunner/mozrunner/runner.py
@@ +69,5 @@
>          Timeout is ignored if interactive was set to True.
>          """
> +        if self.process_handler is not None:
> +            if isinstance(self.process_handler, subprocess.Popen):
> +                self.return_code = self.process_handler.wait()

nit: call it 'returncode' to be consistent with mozprocess and the python subprocess module

@@ +73,5 @@
> +                self.return_code = self.process_handler.wait()
> +            else:
> +                self.process_handler.wait(timeout)
> +                self.return_code = self.process_handler.proc.poll()
> +                if self.process_hander = None:

I think this part we want to leave the way it was, e.g:
    if self.return_code is not None:
        self.process_handler = None

@@ +75,5 @@
> +                self.process_handler.wait(timeout)
> +                self.return_code = self.process_handler.proc.poll()
> +                if self.process_hander = None:
> +                    if self.return_code is None:
> +                        raise Exception("Wait called before start.")

I'd like to make a custom exception for this (e.g RunnerNotStartedError), see http://docs.python.org/2/tutorial/errors.html#user-defined-exceptions for more details (you can basically just create a class that derives from Exception and has nothing but a docstring describing it).

@@ +77,5 @@
> +                if self.process_hander = None:
> +                    if self.return_code is None:
> +                        raise Exception("Wait called before start.")
> +                    else:
> +                        return self.return_code

This is the right idea, but it needs to be outside the outermost if statement. For example:
    if self.process_handler is not None:
        ...
    elif self.return_code is None:
        raise ...
    return self.return_code

@@ +79,5 @@
> +                        raise Exception("Wait called before start.")
> +                    else:
> +                        return self.return_code
> +
> +        return self.return_code

This will cause an error if we skip the if block and never initialize self.return_code. The standard thing to do is to put 'self.return_code = None' in the __init__ method.

Comment 7

[Sebastiaan de Haan [:kyr0]]

Attachment: Attempt to make the changes to the method: mozrunner.wait()

I had some problems with Git generating this patch, but I think it's correct. Please let me know if not, I shall try and fix it right away.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 818882 [details] [diff] [review]
Attempt to make the changes to the method: mozrunner.wait()

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

Thanks! There are a few minor things you'll need to change. Please upload one last patch, carry forward the r+, and I'll handle testing it on try server and then landing if everything looks good.

::: mozrunner/mozrunner/runner.py
@@ +77,5 @@
> +                self.returncode = self.process_handler.wait()
> +            else:
> +                self.process_handler.wait(timeout)
> +                self.returncode = self.process_handler.proc.poll()
> +                if returncode is not None:

this needs to be self.returncode

@@ +80,5 @@
> +                self.returncode = self.process_handler.proc.poll()
> +                if returncode is not None:
> +                    self.process_handler = None
> +        elif self.returncode is None:
> +            raise RunnerNotStartedError("Wait called before started")

nit: change the message to "Wait called before runner started"

@@ +85,1 @@
>          else:

drop the else clause. In general it is good coding practice to avoid having nothing but a return statement in an else clause. In this case we actually want to return self.returncode even if the first 'if' statement is hit.

Comment 9

[Sebastiaan de Haan [:kyr0]]

Attachment: Made changes to the method mozrunner.wait()

Final patch as requested.

Comment 10

[Andrew Halberstadt [:ahal]]

(In reply to Sebastiaan de Haan from comment #9)
> Created attachment 819144 [details] [diff] [review]
> Made changes to the method mozrunner.wait()
> 
> Final patch as requested.

Thanks! I pushed the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=51d7f348c28c

We'll need to wait a few hours for the results to come in.

Comment 11

[Andrew Halberstadt [:ahal]]

The last push had some build issues that look infra related. Second attempt, linux only this time:
https://tbpl.mozilla.org/?tree=Try&rev=0f352d5be580

Comment 12

[Andrew Halberstadt [:ahal]]

Checked in: https://github.com/mozilla/mozbase/commit/2d0ea35a3761980b24c994262878aa35dbaa6664

Thanks Sebastiaan!