962495 - [mozrunner] Calling runner.start() with a timeout or outputTimeout, it doesn't handle stopped process

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: runner.py

With my work on bug 960084 I have seen that mozrunner doesn't correctly handle the timeout, which can be given in runner.start(). Even the process has been killed, mozrunner assumes it is still running.

Attached you will find an example, which can be integrated later as test for mozrunner once the patch on bug 960084 has been landed.

Steps:
1. Export BROWSER_PATH and set it to your firefox binary
2. Run 'python runner.py'

The execution should not assert.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The same also applies to outputTimeout where it also fails to detect the stopped process.

Comment 2

[Jarek Śmiejczak [:jotes]]

Attachment: 0001-Bug-962495-fixed-is_running-behaviour-in-mozrunner.-.patch

Currently we support only 2 types of process_handler - Popen, ProcessHandler. 

Another, more clean method would be to implement poll() method in ProcessHandler to remove typecheck and just use natural duck-typing. I wasn't sure if we should go this way.

TIA for review :)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jarek Śmiejczak from comment #2)
> Another, more clean method would be to implement poll() method in
> ProcessHandler to remove typecheck and just use natural duck-typing. I
> wasn't sure if we should go this way.

Right. It's not only the situation for poll() but also for wait() and kill(), which I have fixed a couple of moments ago on bug 965183. So we should really move this into mozprocess. William, what is your opinion on that?

Comment 4

[William Lachance (:wlach)]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Jarek Śmiejczak from comment #2)
> > Another, more clean method would be to implement poll() method in
> > ProcessHandler to remove typecheck and just use natural duck-typing. I
> > wasn't sure if we should go this way.
> 
> Right. It's not only the situation for poll() but also for wait() and
> kill(), which I have fixed a couple of moments ago on bug 965183. So we
> should really move this into mozprocess. William, what is your opinion on
> that?

Looks like ProcessHandler / ProcessHandlerMixin already has wait and kill. Adding poll would be a natural extension.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8365619 [details] [diff] [review]
0001-Bug-962495-fixed-is_running-behaviour-in-mozrunner.-.patch

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

::: mozrunner/mozrunner/base.py
@@ +123,5 @@
> +        if isinstance(self.process_handler, subprocess.Popen):
> +            return self.process_handler.poll() is None
> +        elif isinstance(self.process_handler, ProcessHandler):
> +            return self.process_handler.proc.poll() is None
> +        return False

I think it would be good if we would check for a valid process_handler first, so that we do not have to mix up the conditions. Also it would make it easier later if we are going to have such a wrapper in ProcessHandler itself.

For now I would be fine with it. But I would like to have another review by Andrew.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8365619 [details] [diff] [review]
0001-Bug-962495-fixed-is_running-behaviour-in-mozrunner.-.patch

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

Looks good, though I guess if we are adding a poll() method to mozprocess in the other bug, we might as well block on that.

Comment 7

[Jarek Śmiejczak [:jotes]]

We can create new bug and set it as a blocker for this one or I can implement it now. I'm not sure who should take decide on that matter.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I filed bug 965326 for the follow-up work.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master:
https://github.com/mozilla/mozbase/commit/b7f3c9cc143770446f28c66ceb3da6412108e15b

Thanks Jarek!