SetIOAllowed when stopping worker audio threads

SetIOAllowed when stopping worker audio threads

Stopping a thread is considered "IO" since it "may" take a long time.  In the case with these audio threads, they are already idle, so stopping should not block.

BUG=chromium-os:11110
TEST=(none)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72235

[davejcool, 2011-01-21 20:23:46]

This should fix it.

[willchan no longer on Chromium, 2011-01-21 20:31:26]

Every ScopedAllowIO should be accompanied with a link to the bug discussion
on why it's valid.

On Fri, Jan 21, 2011 at 12:23 PM, <davej@chromium.org> wrote:

> Reviewers: scherkus, Nikita Kostylev,
>
> Message:
> This should fix it.
>
> Description:
> SetIOAllowed when stopping worker audio threads
>
> Stopping a thread is considered "IO" since it "may" take a long time.  In
> the
> case with these audio threads, they are already idle, so stopping should
> not
> block.
>
> BUG=chrome-os:11110
> TEST=(none)
>
> Please review this at http://codereview.chromium.org/6254016/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome/browser/chromeos
>
> Affected files:
>  M chrome/browser/chromeos/audio_mixer_alsa.cc
>  M chrome/browser/chromeos/audio_mixer_pulse.cc
>
>
> Index: chrome/browser/chromeos/audio_mixer_alsa.cc
> diff --git a/chrome/browser/chromeos/audio_mixer_alsa.cc
> b/chrome/browser/chromeos/audio_mixer_alsa.cc
> index
>
01c22bdae36451a27c4bd454591b0db0a8af680d..af6f2dbff40a1d77494dad38ce62a9e9fd3d455c
> 100644
> --- a/chrome/browser/chromeos/audio_mixer_alsa.cc
> +++ b/chrome/browser/chromeos/audio_mixer_alsa.cc
> @@ -8,6 +8,7 @@
>
>  #include "base/logging.h"
>  #include "base/task.h"
> +#include "base/threading/thread_restrictions.h"
>  #include "chrome/browser/browser_process.h"
>  #include "chrome/browser/browser_thread.h"
>  #include "chrome/browser/prefs/pref_service.h"
> @@ -51,6 +52,10 @@ AudioMixerAlsa::AudioMixerAlsa()
>  AudioMixerAlsa::~AudioMixerAlsa() {
>   FreeAlsaMixer();
>   if (thread_ != NULL) {
> +    // A ScopedAllowIO object is required to join the thread when calling
> Stop.
> +    // There should be nothing happening on the thread at the time of this
> call.
> +    base::ThreadRestrictions::ScopedAllowIO allow_io_for_thread_join;
> +
>     thread_->Stop();
>     thread_.reset();
>   }
> Index: chrome/browser/chromeos/audio_mixer_pulse.cc
> diff --git a/chrome/browser/chromeos/audio_mixer_pulse.cc
> b/chrome/browser/chromeos/audio_mixer_pulse.cc
> index
>
ddff1358278ed6cc0f4b4765917e2ca5a1522162..c2f760098c65fb0d77d519cc8df4e2efb62976ce
> 100644
> --- a/chrome/browser/chromeos/audio_mixer_pulse.cc
> +++ b/chrome/browser/chromeos/audio_mixer_pulse.cc
> @@ -8,6 +8,7 @@
>
>  #include "base/logging.h"
>  #include "base/task.h"
> +#include "base/threading/thread_restrictions.h"
>
>  namespace chromeos {
>
> @@ -59,6 +60,10 @@ AudioMixerPulse::AudioMixerPulse()
>  AudioMixerPulse::~AudioMixerPulse() {
>   PulseAudioFree();
>   if (thread_ != NULL) {
> +    // A ScopedAllowIO object is required to join the thread when calling
> Stop.
> +    // There should be nothing happening on the thread at the time of this
> call.
> +    base::ThreadRestrictions::ScopedAllowIO allow_io_for_thread_join;
> +
>     thread_->Stop();
>     thread_.reset();
>   }
>
>
>

[davejcool, 2011-01-21 21:56:09]

Referencing Issue 11110 in the comments.

The thread's MessageLoop should already be empty at the time Stop() is called. 
To assert this, I could not find a method to query if the MessageLoop's task
queue was empty.  I suppose I could add counter and DCHECK(!in_a_task_) before
thread_->Stop() is called?  But the counter code would execute even in release.

[willchan no longer on Chromium, 2011-01-21 22:04:02]

I am in favor of adding a method to MessageLoop that exposes whether or not
the task queue is empty. We would have to be very careful to document that
this is not to be used for scheduling purposes, since a temporary idle state
is no indicator of future idleness. I think you can check in the code as is.
I'm going to see what Darin thinks of such a change to MessageLoop for
assertion purposes.

On Fri, Jan 21, 2011 at 1:56 PM, <davej@chromium.org> wrote:

> Referencing Issue 11110 in the comments.
>
> The thread's MessageLoop should already be empty at the time Stop() is
> called.
> To assert this, I could not find a method to query if the MessageLoop's
> task
> queue was empty.  I suppose I could add counter and DCHECK(!in_a_task_)
> before
> thread_->Stop() is called?  But the counter code would execute even in
> release.
>
>
> http://codereview.chromium.org/6254016/
>

[davejcool, 2011-01-21 22:26:27]

On 2011/01/21 22:04:02, willchan wrote:
> I am in favor of adding a method to MessageLoop that exposes whether or not
> the task queue is empty. We would have to be very careful to document that
> this is not to be used for scheduling purposes, since a temporary idle state
> is no indicator of future idleness. I think you can check in the code as is.
> I'm going to see what Darin thinks of such a change to MessageLoop for
> assertion purposes.
> 
> On Fri, Jan 21, 2011 at 1:56 PM, <mailto:davej@chromium.org> wrote:
> 
> > Referencing Issue 11110 in the comments.
> >
> > The thread's MessageLoop should already be empty at the time Stop() is
> > called.
> > To assert this, I could not find a method to query if the MessageLoop's
> > task
> > queue was empty.  I suppose I could add counter and DCHECK(!in_a_task_)
> > before
> > thread_->Stop() is called?  But the counter code would execute even in
> > release.
> >
> >
> > http://codereview.chromium.org/6254016/
> >

Thanks.  Putting an is_idle() check inside Stop() I think is a great idea, as it
will verify all the other places Stop() is called as well, not just this one.

[Nikita (slow), 2011-01-24 15:05:25]

LGTM
ui_tests are now fine.