Move BindToCurrentLoop from media/base/ to base/

Move BindToCurrentLoop from media/base/ to base/

PostTaskAndReplyWithResult works great when you need to call a
synchronous method on another thread, and get the reply back on the same
thread. But it doesn't work when you need to call an asynchronous method
on another thread, and pass in a callback that will be run on the same
thread.

media::BindToCurrentLoop allows this to be done cleanly, and would be
useful throughout Chrome for simplifying thread hops. For example, it
allows the following code:

    static void RepostOnThread(
        const scoped_refptr<SingleThreadTaskRunner>& task_runner,
        A a, B b, C c, X x, Y y, Z z) {
      task_runner->PostTask(FROM_HERE, Base::Bind(&TheActualCallback,
                                                  a, b, c, x, y, z));
    }
    ...
    RepliesOnWrongThread(base::Bind(&RepostOnThread,
                                    MessageLoop::current()->task_runner(),
                                    a, b, c));

to be replaced with just:

    RepliesOnWrongThread(base::BindToCurrentLoop(base::Bind(&TheActualCallback,
                                                            a, b, c)));

This patch moves it to base/ so it can be used throughout Chrome.

BUG=432381

[johnme, 2015-04-22 16:30:20]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 16:30:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1083883003/1

[commit-bot: I haz the power, 2015-04-22 17:00:30]

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

[commit-bot: I haz the power, 2015-04-22 17:00:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2015-04-22 17:07:32]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 17:10:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1083883003/20001

[johnme, 2015-04-22 18:07:00]

johnme@chromium.org changed reviewers:
+ brettw@chromium.org

[commit-bot: I haz the power, 2015-04-22 18:35:54]

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

[commit-bot: I haz the power, 2015-04-22 18:35:55]

Dry run: This issue passed the CQ dry run.

[danakj, 2015-04-23 16:25:57]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-04-23 16:25:58]

I have concerns with propagating this pattern. This makes the deletion of the
Closure to be thread-racey. It may be deleted on the orignal "Current" thread or
the thread being posted from first.

If the closure holds any refcounted objects inside of it, those objects
destruction happens on a racey thread.

I've wanted to use this exact class before and received fair warnings and
guidance to not use it (or similar general/templated solutions) elsewhere, and
instead write something more specific for the use case that would be clear and
avoid such hidden issues.

[johnme, 2015-04-23 18:24:22]

On 2015/04/23 16:25:58, danakj wrote:
> I have concerns with propagating this pattern. This makes the deletion of the
> Closure to be thread-racey. It may be deleted on the orignal "Current" thread
or
> the thread being posted from first.
> 
> If the closure holds any refcounted objects inside of it, those objects
> destruction happens on a racey thread.

Hum. I wonder if we can assert at compile time that any refcounted objects must
derive from RefCountedThreadSafe<Foo, BrowserThread::DeleteOnIOThread> or
similar?

Or perhaps BindToCurrentLoop should ensure the Closure is always deleted on the
original ("current") thread, like PostTaskAndReply does. That's probably safest.

> I've wanted to use this exact class before and received fair warnings and
> guidance to not use it (or similar general/templated solutions) elsewhere, and
> instead write something more specific for the use case that would be clear and
> avoid such hidden issues.

Calling a method on another thread, then getting an async response back on the
original thread, is such a common need in Chrome, that it would really help to
have a proper primitive for this.

[danakj, 2015-04-23 18:52:46]

On Thu, Apr 23, 2015 at 11:24 AM, <johnme@chromium.org> wrote:

> On 2015/04/23 16:25:58, danakj wrote:
>
>> I have concerns with propagating this pattern. This makes the deletion of
>> the
>> Closure to be thread-racey. It may be deleted on the orignal "Current"
>> thread
>>
> or
>
>> the thread being posted from first.
>>
>
>  If the closure holds any refcounted objects inside of it, those objects
>> destruction happens on a racey thread.
>>
>
> Hum. I wonder if we can assert at compile time that any refcounted objects
> must
> derive from RefCountedThreadSafe<Foo, BrowserThread::DeleteOnIOThread> or
> similar?
>

I don't think so, they can be buried in arbitrarily many levels of Closures
and it doesn't matter until you try to PostTask the last closure.

Or perhaps BindToCurrentLoop should ensure the Closure is always deleted on
> the
> original ("current") thread, like PostTaskAndReply does. That's probably
> safest.


This would be good to do.

 I've wanted to use this exact class before and received fair warnings and
>> guidance to not use it (or similar general/templated solutions)
>> elsewhere, and
>> instead write something more specific for the use case that would be
>> clear and
>> avoid such hidden issues.
>>
>
> Calling a method on another thread, then getting an async response back on
> the
> original thread, is such a common need in Chrome, that it would really
> help to
> have a proper primitive for this.
>

The places that I know of that need to post and get an async reply don't
generally do so by passing over a Callback that should run on a mystery
thread. The other side knows what thread the request came from and just
Posts its reply back from there.

This is much more explicit and I would not want to replace these with such
a Callback.

I can see that the media code uses this as a design pattern extensively,
but I'd need some good convincing that we want to use this more widely.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[johnme, 2015-04-24 18:48:41]

On 2015/04/23 18:52:46, danakj wrote:
> On Thu, Apr 23, 2015 at 11:24 AM, johnme wrote:
> > On 2015/04/23 16:25:58, danakj wrote:
> > > If the closure holds any refcounted objects inside of it, those objects
> > > destruction happens on a racey thread.
> >
> > perhaps BindToCurrentLoop should ensure the Closure is always deleted on
> > the
> > original ("current") thread, like PostTaskAndReply does. That's probably
> > safest.
> 
> This would be good to do.

Done, PTAL: https://codereview.chromium.org/1082113004

> > Calling a method on another thread, then getting an async response back on
> > the
> > original thread, is such a common need in Chrome, that it would really
> > help to
> > have a proper primitive for this.
> >
> 
> The places that I know of that need to post and get an async reply don't
> generally do so by passing over a Callback that should run on a mystery
> thread. The other side knows what thread the request came from and just
> Posts its reply back from there.
> 
> This is much more explicit and I would not want to replace these with such
> a Callback.

An example of where this is useful is
https://codereview.chromium.org/1099093003/diff/40001/chrome/browser/push_mes...
which is a class entirely on the UI thread, but in that patch I twice needed
to call asynchronous methods which must be called on the IO thread and which
return their response by calling a callback on the IO thread.

The other side is content/public layer code that happily lives on the IO
thread, and is normally used from the IO thread, and it doesn't make sense
to tightly couple that code to its callers such that it knows on which
thread to run the callback.

The normal result in such situation is that the calling class ends up
gaining some IO thread methods in addition to its UI thread methods, and
suddenly you have to be much more careful about what thread everything is
running on. Instead, the approach taken in the linked patch using
BindToCurrentLoop avoids this, and allows PushMessagingServiceImpl to
remain entirely on the UI thread, which seems significantly more legible.

[danakj, 2015-04-29 21:05:29]

On Fri, Apr 24, 2015 at 11:48 AM, <johnme@chromium.org> wrote:

> On 2015/04/23 18:52:46, danakj wrote:
>
>> On Thu, Apr 23, 2015 at 11:24 AM, johnme wrote:
>> > On 2015/04/23 16:25:58, danakj wrote:
>> > > If the closure holds any refcounted objects inside of it, those
>> objects
>> > > destruction happens on a racey thread.
>> >
>> > perhaps BindToCurrentLoop should ensure the Closure is always deleted on
>> > the
>> > original ("current") thread, like PostTaskAndReply does. That's probably
>> > safest.
>>
>
>  This would be good to do.
>>
>
> Done, PTAL: https://codereview.chromium.org/1082113004
>
>  > Calling a method on another thread, then getting an async response back
>> on
>> > the
>> > original thread, is such a common need in Chrome, that it would really
>> > help to
>> > have a proper primitive for this.
>> >
>>
>
>  The places that I know of that need to post and get an async reply don't
>> generally do so by passing over a Callback that should run on a mystery
>> thread. The other side knows what thread the request came from and just
>> Posts its reply back from there.
>>
>
>  This is much more explicit and I would not want to replace these with such
>> a Callback.
>>
>
> An example of where this is useful is
>
>
https://codereview.chromium.org/1099093003/diff/40001/chrome/browser/push_mes...
> which is a class entirely on the UI thread, but in that patch I twice
> needed
> to call asynchronous methods which must be called on the IO thread and
> which
> return their response by calling a callback on the IO thread.


> The other side is content/public layer code that happily lives on the IO
> thread, and is normally used from the IO thread, and it doesn't make sense
> to tightly couple that code to its callers such that it knows on which
> thread to run the callback.
>
> The normal result in such situation is that the calling class ends up
> gaining some IO thread methods in addition to its UI thread methods, and
> suddenly you have to be much more careful about what thread everything is
> running on. Instead, the approach taken in the linked patch using
> BindToCurrentLoop avoids this, and allows PushMessagingServiceImpl to
> remain entirely on the UI thread, which seems significantly more legible.
>

Thanks for the example. I'm not used to code building general cross-thread
services like that.

I actually think that it's nice to be explicit about threads and as long as
you put thread names in the function names having multiple threads in a
class is not as scary as it would seem. Functions OnSomeOtherThread do
nothing but post-task to other methods in the class.

That said, as I said on the CL for fixing BindToCurrentLoop, I think the
current thing is awwwfully general, and maybe we could do something a
little more targetted/safe/simple for async replies? I have to admit I'd
probably prefer if we did such things closer to that places that want to
send these async replies even if its duplicated, as they can be even more
explicit/less general that way.


>
> https://codereview.chromium.org/1083883003/
>


On Fri, Apr 24, 2015 at 11:48 AM, <johnme@chromium.org> wrote:

> On 2015/04/23 18:52:46, danakj wrote:
>
>> On Thu, Apr 23, 2015 at 11:24 AM, johnme wrote:
>> > On 2015/04/23 16:25:58, danakj wrote:
>> > > If the closure holds any refcounted objects inside of it, those
>> objects
>> > > destruction happens on a racey thread.
>> >
>> > perhaps BindToCurrentLoop should ensure the Closure is always deleted on
>> > the
>> > original ("current") thread, like PostTaskAndReply does. That's probably
>> > safest.
>>
>
>  This would be good to do.
>>
>
> Done, PTAL: https://codereview.chromium.org/1082113004
>
>  > Calling a method on another thread, then getting an async response back
>> on
>> > the
>> > original thread, is such a common need in Chrome, that it would really
>> > help to
>> > have a proper primitive for this.
>> >
>>
>
>  The places that I know of that need to post and get an async reply don't
>> generally do so by passing over a Callback that should run on a mystery
>> thread. The other side knows what thread the request came from and just
>> Posts its reply back from there.
>>
>
>  This is much more explicit and I would not want to replace these with such
>> a Callback.
>>
>
> An example of where this is useful is
>
>
https://codereview.chromium.org/1099093003/diff/40001/chrome/browser/push_mes...
> which is a class entirely on the UI thread, but in that patch I twice
> needed
> to call asynchronous methods which must be called on the IO thread and
> which
> return their response by calling a callback on the IO thread.
>
> The other side is content/public layer code that happily lives on the IO
> thread, and is normally used from the IO thread, and it doesn't make sense
> to tightly couple that code to its callers such that it knows on which
> thread to run the callback.
>
> The normal result in such situation is that the calling class ends up
> gaining some IO thread methods in addition to its UI thread methods, and
> suddenly you have to be much more careful about what thread everything is
> running on. Instead, the approach taken in the linked patch using
> BindToCurrentLoop avoids this, and allows PushMessagingServiceImpl to
> remain entirely on the UI thread, which seems significantly more legible.
>
> https://codereview.chromium.org/1083883003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-14 19:58:16]

danakj@chromium.org changed reviewers:
- brettw@chromium.org

[danakj, 2016-04-14 19:58:17]

I've continued to think about this and talk about it with others and I sorry but
I don't think that we should put this in base/.

The BindToCurrentLoop is syntactic sugar to bind a task runner into a callback,
and I think it's really not that bad (and I'd generally prefer) for people to
pass a task runner to the method they want to reply from rather than hiding it
inside a Callback. I was hoping this construct could provide some added value in
terms of more safety/leak-prevention but that turned out to be not the case.