Associate threads in reraiser_thread.RunAsync with the calling ThreadGroup

Associate threads in reraiser_thread.RunAsync with the calling ThreadGroup

This mainly affects devil.android.decorators. Before the change a
decorated method called by a RunAsync thread would have a separate
timeout created for it. With this change, methods within RunAsync detect
that there already exists an outer timeout thread, and do not have a
separate timeout applied to them.

BUG=

Committed: https://crrev.com/1ddba0da5c8684a016f0ff853614d1dd6d9d178e
Cr-Commit-Position: refs/heads/master@{#351947}

[agrieve, 2015-10-01 14:58:41]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org

[agrieve, 2015-10-01 14:59:24]

On 2015/10/01 14:58:41, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:jbudorick@chromium.org

plz review. Tested that this fixes https://codereview.chromium.org/1371563002/
:)

[jbudorick, 2015-10-01 19:28:45]

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/r...
File build/android/devil/utils/reraiser_thread.py (right):

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/r...
build/android/devil/utils/reraiser_thread.py:124:
self.blocked_parent_thread_group = CurrentThreadGroup()
sanity check: calling CurrentThreadGroup() here should return self, right?

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
File build/android/devil/utils/timeout_retry.py (right):

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:158: thread_group.StartAll()
Should this set will_block=True? We join immediately after...

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:167: # Timeouts already get their
stacks logged.
though the stack is only logged once, after all tries.

[agrieve, 2015-10-01 19:44:28]

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/r...
File build/android/devil/utils/reraiser_thread.py (right):

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/r...
build/android/devil/utils/reraiser_thread.py:124:
self.blocked_parent_thread_group = CurrentThreadGroup()
On 2015/10/01 19:28:44, jbudorick wrote:
> sanity check: calling CurrentThreadGroup() here should return self, right?

Nope. StartAll is called from a thread that isn't a part of self (those threads
aren't even running yet)

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
File build/android/devil/utils/timeout_retry.py (right):

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:158: thread_group.StartAll()
On 2015/10/01 19:28:44, jbudorick wrote:
> Should this set will_block=True? We join immediately after...

Added.

I don't think the information will be used in this case though, since
CurrentTimeoutThreadGroup returns the first-hit group.

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:167: # Timeouts already get their
stacks logged.
On 2015/10/01 19:28:44, jbudorick wrote:
> though the stack is only logged once, after all tries.

The error_log_func I added just so that scary stacks wouldn't appear when
running unit tests.

This comment I added, but I've tried to match the existing behaviour (which was
accomplished by mutating self._expired from within a getter...). I don't think
it's a big deal to also re-log timeouts here, so if that's what you're getting
at, then I can delete this special case.

[jbudorick, 2015-10-01 20:37:13]

lgtm w/ nits

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
File build/android/devil/utils/timeout_retry.py (right):

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:158: thread_group.StartAll()
On 2015/10/01 19:44:27, agrieve wrote:
> On 2015/10/01 19:28:44, jbudorick wrote:
> > Should this set will_block=True? We join immediately after...
> 
> Added.
> 
> I don't think the information will be used in this case though, since
> CurrentTimeoutThreadGroup returns the first-hit group.

Ah. I wasn't sure.

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:167: # Timeouts already get their
stacks logged.
On 2015/10/01 19:44:27, agrieve wrote:
> On 2015/10/01 19:28:44, jbudorick wrote:
> > though the stack is only logged once, after all tries.
> 
> The error_log_func I added just so that scary stacks wouldn't appear when
> running unit tests.

Definitely on board with not logging scary stacks when they aren't necessary.

> 
> This comment I added, but I've tried to match the existing behaviour (which
was
> accomplished by mutating self._expired from within a getter...). I don't think
> it's a big deal to also re-log timeouts here, so if that's what you're getting
> at, then I can delete this special case.

I just meant that this was previously logging those timeouts on each try. It now
will only log a timeout at the end. I think that's ok, I just wanted to point it
out.

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:170: except:
nit: I think this should be Exception s.t. we at least don't catch and retry on
KeyboardInterrupt. (We haven't done this elsewhere and we weren't doing it
previously here, obviously, but I've had telemetry folks point it out to me on
other reviews and it seems like a good idea.)

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:173: _LogLastException(thread_name,
num_try, retries + 1, error_log_func)
optional nit: should we catch the exception with a variable and pass it in
rather than just pulling from traceback? If multiple threads throw exceptions in
separate calls to timeout_retry.Run, you could conceivably wind up logging a
different exception than the one you caught.

(i think)

[agrieve, 2015-10-02 00:19:45]

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
File build/android/devil/utils/timeout_retry.py (right):

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:167: # Timeouts already get their
stacks logged.
On 2015/10/01 20:37:12, jbudorick wrote:
> On 2015/10/01 19:44:27, agrieve wrote:
> > On 2015/10/01 19:28:44, jbudorick wrote:
> > > though the stack is only logged once, after all tries.
> > 
> > The error_log_func I added just so that scary stacks wouldn't appear when
> > running unit tests.
> 
> Definitely on board with not logging scary stacks when they aren't necessary.
> 
> > 
> > This comment I added, but I've tried to match the existing behaviour (which
> was
> > accomplished by mutating self._expired from within a getter...). I don't
think
> > it's a big deal to also re-log timeouts here, so if that's what you're
getting
> > at, then I can delete this special case.
> 
> I just meant that this was previously logging those timeouts on each try. It
now
> will only log a timeout at the end. I think that's ok, I just wanted to point
it
> out.

Ahaaaa, okay.

This change I did on purpose, but it's not quite as you say. It's logging them
every time *except* the end.

The exception that is rethrown should be logged / handled by the calling
function.

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:170: except:
On 2015/10/01 20:37:12, jbudorick wrote:
> nit: I think this should be Exception s.t. we at least don't catch and retry
on
> KeyboardInterrupt. (We haven't done this elsewhere and we weren't doing it
> previously here, obviously, but I've had telemetry folks point it out to me on
> other reviews and it seems like a good idea.)

Agree! Done.

https://codereview.chromium.org/1376473006/diff/1/build/android/devil/utils/t...
build/android/devil/utils/timeout_retry.py:173: _LogLastException(thread_name,
num_try, retries + 1, error_log_func)
On 2015/10/01 20:37:12, jbudorick wrote:
> optional nit: should we catch the exception with a variable and pass it in
> rather than just pulling from traceback? If multiple threads throw exceptions
in
> separate calls to timeout_retry.Run, you could conceivably wind up logging a
> different exception than the one you caught.
> 
> (i think)

I would much prefer passing in the exception, but I don't actually see a python
API that lets you do that :(.

I suspect traceback.format_exc() is thread-local, so don't think there's any bug
at least.

[agrieve, 2015-10-02 00:20:05]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-02 00:20:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376473006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376473006/20001

[agrieve, 2015-10-02 00:20:30]

The CQ bit was unchecked by agrieve@chromium.org

[agrieve, 2015-10-02 00:25:08]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-10-02 00:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1376473006/#ps40001
(title: "add Exception to exepct")

[commit-bot: I haz the power, 2015-10-02 00:25:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376473006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376473006/40001

[commit-bot: I haz the power, 2015-10-02 00:34:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 00:34:05]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[agrieve, 2015-10-02 01:07:59]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:08:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376473006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376473006/40001

[agrieve, 2015-10-02 01:10:32]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-10-02 01:10:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1376473006/#ps60001
(title: "rebase due to patch failure on win bot")

[commit-bot: I haz the power, 2015-10-02 01:11:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376473006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376473006/60001

[commit-bot: I haz the power, 2015-10-02 01:21:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:21:57]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[agrieve, 2015-10-02 02:22:03]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-02 02:22:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376473006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376473006/60001

[commit-bot: I haz the power, 2015-10-02 03:09:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-02 03:09:48]

Patchset 4 (id:??) landed as
https://crrev.com/1ddba0da5c8684a016f0ff853614d1dd6d9d178e
Cr-Commit-Position: refs/heads/master@{#351947}