Add timeouts for shell/checkCmd

RESOLVED FIXED in mozilla17

Status

Testing
Mozbase
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mdas, Unassigned)

Tracking

Trunk
mozilla17
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 651855 [details] [diff] [review]
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.
Attachment #651855 - Flags: review?(jmaher)
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.
Created attachment 651860 [details] [diff] [review]
support for devicemanagerADB v0.2

addressing ahal's comments by raising NotImplementedError for sendCmds/shell calls with timeout.
Attachment #651855 - Attachment is obsolete: true
Attachment #651855 - Flags: review?(jmaher)
Attachment #651860 - Flags: review?(jmaher)
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.
Attachment #651860 - Flags: review?(jmaher) → review-
(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 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.
Attachment #651860 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/f2a4e69f264e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.