782749 - Add timeouts for shell/checkCmd

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: support for devicemanagerADB

We'd like to add the ability to timeout a shell command to a device.

In the attached patch, Iadded the timeout parameter for the shell command in devicemanager.py, and full support for timeouts in devicemanagerADB.py. I've tested the checkCmd and shell commands against my pandaboard and it works as expected.

I don't really use devicemanagerSUT.py, so all I did was allow it to accept the timeout parameter, but it doesn't have support yet. If requested, I can get that working here too, but it's a bit of a detour.

Comment 1

[Andrew Halberstadt [:ahal]]

Perhaps we should raise a NotImplementedError. I can envision this getting forgotten about and then when we start using the SUT agent exclusively someone will be confused.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: support for devicemanagerADB v0.2

addressing ahal's comments by raising NotImplementedError for sendCmds/shell calls with timeout.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 651860 [details] [diff] [review]
support for devicemanagerADB v0.2

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

A few comments below that concern me.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +118,4 @@
>      args.extend(["shell", cmdline])
>      proc = subprocess.Popen(args,
>                              stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +    if timeout:

can we put an upper bound on timeout and ensure that it is a integer value?

@@ +130,1 @@
>      (stdout, stderr) = proc.communicate()

does this work if the process has died?

@@ +739,5 @@
> +            ret_code = proc.poll()
> +        if ret_code == None:
> +            proc.kill()
> +            raise DMError("Timeout exceeded for checkCmd call")
> +        return ret_code

why do we run subprocess.popen conditionally?  I don't understand what we are doing in this new block of code.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Joel Maher (:jmaher) from comment #3)
> Comment on attachment 651860 [details] [diff] [review]
> support for devicemanagerADB v0.2
> 
> Review of attachment 651860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few comments below that concern me.
> 
> ::: mozdevice/mozdevice/devicemanagerADB.py
> @@ +118,4 @@
> >      args.extend(["shell", cmdline])
> >      proc = subprocess.Popen(args,
> >                              stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> > +    if timeout:
> 
> can we put an upper bound on timeout and ensure that it is a integer value?

We spoke about this offline, and resolved to just ensure that it is an integer value.

> 
> @@ +130,1 @@
> >      (stdout, stderr) = proc.communicate()
> 
> does this work if the process has died?
 
Yes, communicate() will return the output if the process dies or completes.

> 
> @@ +739,5 @@
> > +            ret_code = proc.poll()
> > +        if ret_code == None:
> > +            proc.kill()
> > +            raise DMError("Timeout exceeded for checkCmd call")
> > +        return ret_code
> 
> why do we run subprocess.popen conditionally?  I don't understand what we
> are doing in this new block of code.

If a timeout is specified, then we cannot use subprocess.check_call, since this function will only return once the shell command completes (http://docs.python.org/library/subprocess.html#subprocess.check_call). To replicate check_call's behaviour with a timeout, I execute the shell command using Popen, polling it to see if it completes. If it does, I return the return code (which is the behaviour of check_call), otherwise, we throw the timeout error and kill the proc.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 651860 [details] [diff] [review]
support for devicemanagerADB v0.2

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

changing to r+ based on irc chat and additional comment in the bug.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/f2a4e69f264e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2a4e69f264e