[android] Add timeout_retry module to pylib/utils/.

[android] Add timeout_retry module to pylib/utils/.

BUG=318387
TEST=None
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235776

[craigdh, 2013-11-13 21:46:37]

ptal.

[craigdh, 2013-11-14 22:12:43]

On 2013/11/13 21:46:37, craigdh wrote:
> ptal.

ping?

[frankf, 2013-11-14 23:26:05]

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
File build/android/pylib/cmd_helper.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
build/android/pylib/cmd_helper.py:108: def RunWithTimeoutAndRetries(args,
timeout, retries):
Can we combline RunWithTimeoutAndRetries, GetCmdStatusAndOutput, GetCmdOutput
into a single method with optional parameters? The caller can just ignore
output/rc if they don't care.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
File build/android/pylib/utils/timeout_retry.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:5: """A class decorator that adds
timeouts and retries to all public methods."""
Update this

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:15: """Runs the passed function in a
separate thread with timeouts and retries.
clarify that timeout applies to each try not total here as well as in cmd_helper

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:27: ret = [None]
Why is this a list? Add a comment if there'a good reason for it.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:33: retries -= 1
you don't provide defaults for retries or timeout

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:33: retries -= 1
You have a off-by 1 error. retries=1 implies you try twice.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
File build/android/pylib/utils/timeout_retry_unittest.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry_unittest.py:41:
self.assertLess(time.time() - start, 2)
this seems flaky. Is this guranteed to be true under all conditions, however
unlikely?

[craigdh, 2013-11-15 00:26:30]

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
File build/android/pylib/cmd_helper.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
build/android/pylib/cmd_helper.py:108: def RunWithTimeoutAndRetries(args,
timeout, retries):
On 2013/11/14 23:26:05, frankf wrote:
> Can we combline RunWithTimeoutAndRetries, GetCmdStatusAndOutput, GetCmdOutput
> into a single method with optional parameters? The caller can just ignore
> output/rc if they don't care.

While I completely agree this should be done, it's way outside the scope of this
cl. Opened http://crbug.com/319551

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
File build/android/pylib/utils/timeout_retry.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:5: """A class decorator that adds
timeouts and retries to all public methods."""
On 2013/11/14 23:26:05, frankf wrote:
> Update this

Done.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:15: """Runs the passed function in a
separate thread with timeouts and retries.
On 2013/11/14 23:26:05, frankf wrote:
> clarify that timeout applies to each try not total here as well as in
cmd_helper

Done.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:27: ret = [None]
On 2013/11/14 23:26:05, frankf wrote:
> Why is this a list? Add a comment if there'a good reason for it.

There's a good reason. Comment added.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:33: retries -= 1
On 2013/11/14 23:26:05, frankf wrote:
> You have a off-by 1 error. retries=1 implies you try twice.

Sure, I was considering it as tries but that behavior makes more sense with the
word choice.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry.py:33: retries -= 1
On 2013/11/14 23:26:05, frankf wrote:
> you don't provide defaults for retries or timeout

I wasn't intending to guess at defaults as those could vary widely as this is a
general utility function. The caller can feel free to provide defaults at a
higher level.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
File build/android/pylib/utils/timeout_retry_unittest.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry_unittest.py:41:
self.assertLess(time.time() - start, 2)
On 2013/11/14 23:26:05, frankf wrote:
> this seems flaky. Is this guranteed to be true under all conditions, however
> unlikely?

It is probably theoretically possible it could fail... but it's not
realistically going to happen. There is no sleep() occurring with a 0 timeout so
it won't fail as long as the code between those locations takes less than two
seconds to execute which seems a fair assumption. I don't think there's a better
way to verify the timeout time is working.

[frankf, 2013-11-15 23:08:47]

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
File build/android/pylib/cmd_helper.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
build/android/pylib/cmd_helper.py:108: def RunWithTimeoutAndRetries(args,
timeout, retries):
Can you at least match naming convention GetCmd...

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
File build/android/pylib/utils/timeout_retry_unittest.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry_unittest.py:41:
self.assertLess(time.time() - start, 2)
If this is an infinite loop, then it should be sufficient to assert a
TimeoutError is raised. I don't see any value in asserting the exact wallclock
time it took.

On 2013/11/15 00:26:30, craigdh wrote:
> On 2013/11/14 23:26:05, frankf wrote:
> > this seems flaky. Is this guranteed to be true under all conditions, however
> > unlikely?
> 
> It is probably theoretically possible it could fail... but it's not
> realistically going to happen. There is no sleep() occurring with a 0 timeout
so
> it won't fail as long as the code between those locations takes less than two
> seconds to execute which seems a fair assumption. I don't think there's a
better
> way to verify the timeout time is working.

[craigdh, 2013-11-15 23:41:56]

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
File build/android/pylib/cmd_helper.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/cmd_h...
build/android/pylib/cmd_helper.py:108: def RunWithTimeoutAndRetries(args,
timeout, retries):
On 2013/11/15 23:08:47, frankf wrote:
> Can you at least match naming convention GetCmd...

Done.

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
File build/android/pylib/utils/timeout_retry_unittest.py (right):

https://codereview.chromium.org/60043003/diff/80001/build/android/pylib/utils...
build/android/pylib/utils/timeout_retry_unittest.py:41:
self.assertLess(time.time() - start, 2)
On 2013/11/15 23:08:47, frankf wrote:
> If this is an infinite loop, then it should be sufficient to assert a
> TimeoutError is raised. I don't see any value in asserting the exact wallclock
> time it took.

Ok, that's reasonable.

[frankf, 2013-11-16 00:04:26]

lgtm

[commit-bot: I haz the power, 2013-11-18 18:43:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/craigdh@chromium.org/60043003/220001

[commit-bot: I haz the power, 2013-11-18 18:52:13]

Change committed as 235776