Make MessageLoopProxy::current() usable on threads that don't have MessageLoop.

Add SingleThreadTaskRunner::current().

With this change now it is possible to get SingleThreadTaskRunner that corresponds to the current thread. That is useful for threads that don't have corresponding MessageLoop object (e.g. main thread in a plugin).

[willchan no longer on Chromium, 2012-04-24 23:25:09]

On 2012/04/24 22:57:58, sergeyu wrote:

This doesn't quite make sense to me. If your thread doesn't have a MessageLoop,
then you shouldn't be using MessageLoopProxy, right? How is that thread
accepting Tasks then? Shouldn't it be using a TaskRunner instead?

Btw, isn't the plugin process running a MessageLoop on the ipc thread? Is the
ipc thread not the main thread in a plugin process?

[Sergey Ulanov, 2012-04-24 23:35:32]

The goal is to make it possible to run code that relies on
MessageLoopProxy::current() on threads that don't have MessageLoop. E.g. timers
need to be able to schedule tasks on current thread and they are not given task
runner for the current thread explicitly. 

Alternatively we could add SingleThreadTaskRunner::current() but the only
difference between SignleThreadTaskRunner is MessageLoopProxy is that
MessageLoopProxy has current().
Another alternative solution is to pass task-runner to the timer class, but that
would require many changes in that code and everything that depends on it
without much benefit.

On 2012/04/24 23:25:09, willchan wrote:
> On 2012/04/24 22:57:58, sergeyu wrote:
> 
> This doesn't quite make sense to me. If your thread doesn't have a
MessageLoop,
> then you shouldn't be using MessageLoopProxy, right? How is that thread
> accepting Tasks then? Shouldn't it be using a TaskRunner instead?
> 
> Btw, isn't the plugin process running a MessageLoop on the ipc thread? Is the
> ipc thread not the main thread in a plugin process?

[Sergey Ulanov, 2012-04-24 23:42:45]

On 2012/04/24 23:25:09, willchan wrote:
> On 2012/04/24 22:57:58, sergeyu wrote:
> 
> This doesn't quite make sense to me. If your thread doesn't have a
MessageLoop,
> then you shouldn't be using MessageLoopProxy, right? How is that thread
> accepting Tasks then? Shouldn't it be using a TaskRunner instead?
> 
> Btw, isn't the plugin process running a MessageLoop on the ipc thread? Is the
> ipc thread not the main thread in a plugin process?

To clarify a bit more - chromoting runs some of the chromium code inside NPAPI
plugin. Obviously we can't access chromium's message loop from the plugin (and
even if we could - it might be a different version of MessageLoop code). What we
do instead is wrap the interface provided to the plugin by the browser with
MessageLoopProxy: see remoting/base/plugin_message_loop_proxy.h and
remoting::PluginMessageLoopProxy::Delegate implementation in
remoting/host/plugin/host_plugin.cc . That MessageLoopProxy implementation
doesn't depend on MessageLoopProxyImpl or MessageLoop.

[willchan no longer on Chromium, 2012-04-25 00:28:12]

MessageLoopProxy is not meant to be an interface. That's what the TaskRunners
are for. MessageLoopProxy is conceptually tightly integrated with MessageLoops.
I'm not a big fan of changing that.

I think I would be able to judge better if I had a better understanding of what
remoting/ is doing. I will acknowledge that my bias is towards telling you to do
it differently :P But I'd like to understand so I can have a better informed
opinion. What bits of Chromium base/ would you like to use here? It seems like
these base/ changes are primarily motivated by wanting to use base::Timer. Is
that the only part, or do you have more bits of base/ that you'd like to use?

On 2012/04/24 23:42:45, sergeyu wrote:
> On 2012/04/24 23:25:09, willchan wrote:
> > On 2012/04/24 22:57:58, sergeyu wrote:
> > 
> > This doesn't quite make sense to me. If your thread doesn't have a
> MessageLoop,
> > then you shouldn't be using MessageLoopProxy, right? How is that thread
> > accepting Tasks then? Shouldn't it be using a TaskRunner instead?
> > 
> > Btw, isn't the plugin process running a MessageLoop on the ipc thread? Is
the
> > ipc thread not the main thread in a plugin process?
> 
> To clarify a bit more - chromoting runs some of the chromium code inside NPAPI
> plugin. Obviously we can't access chromium's message loop from the plugin (and
> even if we could - it might be a different version of MessageLoop code). What
we
> do instead is wrap the interface provided to the plugin by the browser with
> MessageLoopProxy: see remoting/base/plugin_message_loop_proxy.h and
> remoting::PluginMessageLoopProxy::Delegate implementation in
> remoting/host/plugin/host_plugin.cc . That MessageLoopProxy implementation
> doesn't depend on MessageLoopProxyImpl or MessageLoop.

[Sergey Ulanov, 2012-04-25 01:02:34]

On Tue, Apr 24, 2012 at 5:28 PM, <willchan@chromium.org> wrote:

> MessageLoopProxy is not meant to be an interface. That's what the
> TaskRunners
> are for. MessageLoopProxy is conceptually tightly integrated with
> MessageLoops.

I'm not a big fan of changing that.
>

If MessageLoopProxy is not meant to be an interface then why does it need
to be separate from MessageLoopProxyImpl?
I don't see anything in MessageLoopProxy itself that ties it to MessageLoop
is any way (beside the name). And MessageLoop still creates
MessageLoopProxyImpl - I'm not changing anything in that respect.
Remoting is actually not the only place where MessageLoopProxy is used as
an interface - there is also BrowserThreadMessageLoopProxy that implements
MessageLoopProxy independently of MessageLoop.

Would it make sense to add current() in SingleThreadTaskRunner()?


>
> I think I would be able to judge better if I had a better understanding of
> what
> remoting/ is doing. I will acknowledge that my bias is towards telling you
> to do
> it differently :P But I'd like to understand so I can have a better
> informed
> opinion. What bits of Chromium base/ would you like to use here? It seems
> like
> these base/ changes are primarily motivated by wanting to use base::Timer.
> Is
> that the only part, or do you have more bits of base/ that you'd like to
> use?


What I need to solve right now is to be able to reuse
net::ProxyConfigServiceLinux in the plugin. This code must run on the main
glib thread, so that it's safe to use gconf. That code depends on
base::OneShotTimer that relies on MessageLoop::current().
Beside that there are many cases when current chromoting code is suboptimal
because we don't use timer classes from base/timer.h on the plugin thread.
In some other cases we run code that depends on MessageLoop::current() on a
separate thread just to workaround this dependency (that's not possible
with net::ProxyConfigServiceLinux).


>
>
> On 2012/04/24 23:42:45, sergeyu wrote:
>
>> On 2012/04/24 23:25:09, willchan wrote:
>> > On 2012/04/24 22:57:58, sergeyu wrote:
>> >
>> > This doesn't quite make sense to me. If your thread doesn't have a
>> MessageLoop,
>> > then you shouldn't be using MessageLoopProxy, right? How is that thread
>> > accepting Tasks then? Shouldn't it be using a TaskRunner instead?
>> >
>> > Btw, isn't the plugin process running a MessageLoop on the ipc thread?
>> Is
>>
> the
>
>> > ipc thread not the main thread in a plugin process?
>>
>
>  To clarify a bit more - chromoting runs some of the chromium code inside
>> NPAPI
>> plugin. Obviously we can't access chromium's message loop from the plugin
>> (and
>> even if we could - it might be a different version of MessageLoop code).
>> What
>>
> we
>
>> do instead is wrap the interface provided to the plugin by the browser
>> with
>> MessageLoopProxy: see remoting/base/plugin_message_**loop_proxy.h and
>> remoting::**PluginMessageLoopProxy::**Delegate implementation in
>> remoting/host/plugin/host_**plugin.cc . That MessageLoopProxy
>> implementation
>> doesn't depend on MessageLoopProxyImpl or MessageLoop.
>>
>
>
>
>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>

[willchan no longer on Chromium, 2012-04-25 01:32:06]

On Tue, Apr 24, 2012 at 6:02 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> On Tue, Apr 24, 2012 at 5:28 PM, <willchan@chromium.org> wrote:
>
>> MessageLoopProxy is not meant to be an interface. That's what the
>> TaskRunners
>> are for. MessageLoopProxy is conceptually tightly integrated with
>> MessageLoops.
>
> I'm not a big fan of changing that.
>>
>
> If MessageLoopProxy is not meant to be an interface then why does it need
> to be separate from MessageLoopProxyImpl?
>

Very good question! I've wondered that often myself :)


> I don't see anything in MessageLoopProxy itself that ties it to
> MessageLoop is any way (beside the name). And MessageLoop still creates
> MessageLoopProxyImpl - I'm not changing anything in that respect.
> Remoting is actually not the only place where MessageLoopProxy is used as
> an interface - there is also BrowserThreadMessageLoopProxy that implements
> MessageLoopProxy independently of MessageLoop.
>

I honestly don't see why BrowserThread does this either. I took a glance
and it looks to me like we could just use MessageLoopProxyImpl everywhere.
I think this is a bug, unless we're doing this for testing reasons, which
may have been a good reason before, but now that we have TaskRunners,
there's no reason to do so now. I just asked jam@ and he agrees with me, so
we're going to kill off BrowserThread's subclass. I'm filing a bug.


>
> Would it make sense to add current() in SingleThreadTaskRunner()?
>

That's a good question. I am leaning towards saying that's OK by me. Lemme
think it over. We would probably want to make SingleThreadTaskRunner
set/unset a ThreadLocalPointer in its constructor/destructor, in a scoped
fashion, since they may be nested.



>
>
>>
>> I think I would be able to judge better if I had a better understanding
>> of what
>> remoting/ is doing. I will acknowledge that my bias is towards telling
>> you to do
>> it differently :P But I'd like to understand so I can have a better
>> informed
>> opinion. What bits of Chromium base/ would you like to use here? It seems
>> like
>> these base/ changes are primarily motivated by wanting to use
>> base::Timer. Is
>> that the only part, or do you have more bits of base/ that you'd like to
>> use?
>
>
> What I need to solve right now is to be able to reuse
> net::ProxyConfigServiceLinux in the plugin. This code must run on the main
> glib thread, so that it's safe to use gconf. That code depends on
> base::OneShotTimer that relies on MessageLoop::current().
> Beside that there are many cases when current chromoting code is
> suboptimal because we don't use timer classes from base/timer.h on the
> plugin thread. In some other cases we run code that depends on
> MessageLoop::current() on a separate thread just to workaround this
> dependency (that's not possible with net::ProxyConfigServiceLinux).
>

I think we want to make Timer work with SingleThreadTaskRunner then. This
would require either supporting SingleThreadTaskRunner::current() or
providing another constructor/setter to allow setting a
SingleThreadTaskRunner in Timer, which would add a pointer to each Timer
object. We also may be able to avoid this with template magic, but I'm less
keen on that.

I currently lean towards support SingleThreadTaskRunner::current().


>
>
>>
>>
>> On 2012/04/24 23:42:45, sergeyu wrote:
>>
>>> On 2012/04/24 23:25:09, willchan wrote:
>>> > On 2012/04/24 22:57:58, sergeyu wrote:
>>> >
>>> > This doesn't quite make sense to me. If your thread doesn't have a
>>> MessageLoop,
>>> > then you shouldn't be using MessageLoopProxy, right? How is that thread
>>> > accepting Tasks then? Shouldn't it be using a TaskRunner instead?
>>> >
>>> > Btw, isn't the plugin process running a MessageLoop on the ipc thread?
>>> Is
>>>
>> the
>>
>>> > ipc thread not the main thread in a plugin process?
>>>
>>
>>  To clarify a bit more - chromoting runs some of the chromium code inside
>>> NPAPI
>>> plugin. Obviously we can't access chromium's message loop from the
>>> plugin (and
>>> even if we could - it might be a different version of MessageLoop code).
>>> What
>>>
>> we
>>
>>> do instead is wrap the interface provided to the plugin by the browser
>>> with
>>> MessageLoopProxy: see remoting/base/plugin_message_**loop_proxy.h and
>>> remoting::**PluginMessageLoopProxy::**Delegate implementation in
>>> remoting/host/plugin/host_**plugin.cc . That MessageLoopProxy
>>> implementation
>>> doesn't depend on MessageLoopProxyImpl or MessageLoop.
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>>
>
>

[Sergey Ulanov, 2012-04-25 02:21:14]

Ok, I've changed this CL to add SingleThreadTaskRunner::current(). Let me know
what you think about it.

On 2012/04/25 01:32:06, willchan wrote:
> On Tue, Apr 24, 2012 at 6:02 PM, Sergey Ulanov <mailto:sergeyu@chromium.org>
wrote:
> 
> > On Tue, Apr 24, 2012 at 5:28 PM, <mailto:willchan@chromium.org> wrote:
> >
> >> MessageLoopProxy is not meant to be an interface. That's what the
> >> TaskRunners
> >> are for. MessageLoopProxy is conceptually tightly integrated with
> >> MessageLoops.
> >
> > I'm not a big fan of changing that.
> >>
> >
> > If MessageLoopProxy is not meant to be an interface then why does it need
> > to be separate from MessageLoopProxyImpl?
> >
> 
> Very good question! I've wondered that often myself :)
> 
> 
> > I don't see anything in MessageLoopProxy itself that ties it to
> > MessageLoop is any way (beside the name). And MessageLoop still creates
> > MessageLoopProxyImpl - I'm not changing anything in that respect.
> > Remoting is actually not the only place where MessageLoopProxy is used as
> > an interface - there is also BrowserThreadMessageLoopProxy that implements
> > MessageLoopProxy independently of MessageLoop.
> >
> 
> I honestly don't see why BrowserThread does this either. I took a glance
> and it looks to me like we could just use MessageLoopProxyImpl everywhere.
> I think this is a bug, unless we're doing this for testing reasons, which
> may have been a good reason before, but now that we have TaskRunners,
> there's no reason to do so now. I just asked jam@ and he agrees with me, so
> we're going to kill off BrowserThread's subclass. I'm filing a bug.
> 
> 
> >
> > Would it make sense to add current() in SingleThreadTaskRunner()?
> >
> 
> That's a good question. I am leaning towards saying that's OK by me. Lemme
> think it over. We would probably want to make SingleThreadTaskRunner
> set/unset a ThreadLocalPointer in its constructor/destructor, in a scoped
> fashion, since they may be nested.
> 
> 
> 
> >
> >
> >>
> >> I think I would be able to judge better if I had a better understanding
> >> of what
> >> remoting/ is doing. I will acknowledge that my bias is towards telling
> >> you to do
> >> it differently :P But I'd like to understand so I can have a better
> >> informed
> >> opinion. What bits of Chromium base/ would you like to use here? It seems
> >> like
> >> these base/ changes are primarily motivated by wanting to use
> >> base::Timer. Is
> >> that the only part, or do you have more bits of base/ that you'd like to
> >> use?
> >
> >
> > What I need to solve right now is to be able to reuse
> > net::ProxyConfigServiceLinux in the plugin. This code must run on the main
> > glib thread, so that it's safe to use gconf. That code depends on
> > base::OneShotTimer that relies on MessageLoop::current().
> > Beside that there are many cases when current chromoting code is
> > suboptimal because we don't use timer classes from base/timer.h on the
> > plugin thread. In some other cases we run code that depends on
> > MessageLoop::current() on a separate thread just to workaround this
> > dependency (that's not possible with net::ProxyConfigServiceLinux).
> >
> 
> I think we want to make Timer work with SingleThreadTaskRunner then. This
> would require either supporting SingleThreadTaskRunner::current() or
> providing another constructor/setter to allow setting a
> SingleThreadTaskRunner in Timer, which would add a pointer to each Timer
> object. We also may be able to avoid this with template magic, but I'm less
> keen on that.
> 
> I currently lean towards support SingleThreadTaskRunner::current().
> 
> 
> >
> >
> >>
> >>
> >> On 2012/04/24 23:42:45, sergeyu wrote:
> >>
> >>> On 2012/04/24 23:25:09, willchan wrote:
> >>> > On 2012/04/24 22:57:58, sergeyu wrote:
> >>> >
> >>> > This doesn't quite make sense to me. If your thread doesn't have a
> >>> MessageLoop,
> >>> > then you shouldn't be using MessageLoopProxy, right? How is that thread
> >>> > accepting Tasks then? Shouldn't it be using a TaskRunner instead?
> >>> >
> >>> > Btw, isn't the plugin process running a MessageLoop on the ipc thread?
> >>> Is
> >>>
> >> the
> >>
> >>> > ipc thread not the main thread in a plugin process?
> >>>
> >>
> >>  To clarify a bit more - chromoting runs some of the chromium code inside
> >>> NPAPI
> >>> plugin. Obviously we can't access chromium's message loop from the
> >>> plugin (and
> >>> even if we could - it might be a different version of MessageLoop code).
> >>> What
> >>>
> >> we
> >>
> >>> do instead is wrap the interface provided to the plugin by the browser
> >>> with
> >>> MessageLoopProxy: see remoting/base/plugin_message_**loop_proxy.h and
> >>> remoting::**PluginMessageLoopProxy::**Delegate implementation in
> >>> remoting/host/plugin/host_**plugin.cc . That MessageLoopProxy
> >>> implementation
> >>> doesn't depend on MessageLoopProxyImpl or MessageLoop.
> >>>
> >>
> >>
> >>
> >>
>
http://codereview.chromium.**org/10210008/%3Chttp://codereview.chromium.org/1...>
> >>
> >
> >

[akalin, 2012-04-25 07:08:39]

+Ami, since we talked about TaskRunner and ::current()-like functions
before.

On Wed, Apr 25, 2012 at 10:32 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

>
>> If MessageLoopProxy is not meant to be an interface then why does it need
>> to be separate from MessageLoopProxyImpl?
>>
>
> Very good question! I've wondered that often myself :)
>
> I honestly don't see why BrowserThread does this either. I took a glance
> and it looks to me like we could just use MessageLoopProxyImpl everywhere.
> I think this is a bug, unless we're doing this for testing reasons, which
> may have been a good reason before, but now that we have TaskRunners,
> there's no reason to do so now. I just asked jam@ and he agrees with me,
> so we're going to kill off BrowserThread's subclass. I'm filing a bug.
>

It's just an artifact of how things used to be before TaskRunners.  It's
okay to combine MLP and MLPI now that we have SingleThreadTaskRunner.


>
>> Would it make sense to add current() in SingleThreadTaskRunner()?
>>
>
> That's a good question. I am leaning towards saying that's OK by me. Lemme
> think it over. We would probably want to make SingleThreadTaskRunner
> set/unset a ThreadLocalPointer in its constructor/destructor, in a scoped
> fashion, since they may be nested.
>

I don't like this, actually. :/ If we're combining MLP and MLPI, then we
can still have MLP::current() (since it's no worse than
MessageLoop::current()) but I don't want to introduce a new global-like
object, since it introduces weird action-at-a-distance effects, especially
with reentrancy and possible future implementations of
SingleThreadTaskRunner.  I'd prefer adding a method to base::Timer() or
something, if possible.

I'm on vacation until next week or so, so maybe we can talk about it more
then, unless you want this CL to go in soon.  Ami may have some thoughts
here, also, since he's familiar with similar problems on the google3 side.

[Ami GONE FROM CHROMIUM, 2012-04-25 17:27:55]

drive-by thoughts; feel free to ignore.

My reaction to this CL is to wonder why put current() in SingleThreadTaskRunner
and not further up the hierarchy, the logical conclusion of which is TaskRunner;
is there a principled reason STTR deserves current() more than TR?  
OTOH, having a TaskRunner::current() would encourage just the kind of
overly-optimistic programming errors I was worried about last time I spoke w/
Fred about current(); namely that different layers of library code and
application code all make assumptions about what they can do with a "current"
TaskRunner, including deadlocking it ("surely there's another thread available
to process my work while I wait for it to finish") and being surprised by
concurrency ("surely this queued work will only kick off once I return from my
current function").

From the reviewlog history here & chatting with Sergey, there seem to be two
goals to this CL:
1) Enable use of Timer on a thread that's not running MessageLoop.  This can be
more simply achieved by giving Timer::Start() a new param of a TaskRunner, and
avoid its use of MessageLoop::current() except in a forwarding version of
Start().
2) Allowing plugins to avoid having to maintain their own pointers to the
browser-provided task-running API, by letting them ask for a global "current"
runner.  Having STTR::current() is only useful for that if something in the
plugin calls SetCurrent(), but in that case why put that in STTR instead of in
the plugin's code?

http://chromiumcodereview.appspot.com/10210008/diff/8001/base/single_thread_t...
File base/single_thread_task_runner.cc (right):

http://chromiumcodereview.appspot.com/10210008/diff/8001/base/single_thread_t...
base/single_thread_task_runner.cc:21: return lazy_tls_ptr.Pointer()->Get();
Can this DCHECK the ret value (see the TODO(darin) in MessageLoop::current()'s
impl).

http://chromiumcodereview.appspot.com/10210008/diff/8001/base/single_thread_t...
base/single_thread_task_runner.cc:26: DCHECK(!current() || !new_current);
WDYT of xor instead of !||! ?
DCHECK(current() ^ new_current);

[Sergey Ulanov, 2012-04-25 17:34:17]

Thanks Fred for your input.
I think it's useful to have a static method that returns the task runner for the
current thread even when that thread doesn't have MessageLoop. I think in an
ideal world we would have the following:
 - An abstract TaskRunner interface (we already have that, thanks Fred!)
 - A static function TaskRunner::GetForCurrentThread() that returns the task
runner for the current thread.
 - MessageLoop class (or MessageLoopProxy) implements TaskRunner.
 - No MessageLoop::current().

That's essentially what we have now, except that we use MessageLoop::current(),
and don't have TaskRunner::GetForCurrentThread(). This means all the code that
needs to schedule tasks on the current thread must depend on MessageLoop. The
approach above would allow to create threads that don't have MessageLoop and
still be able to run code that needs to access current task runner. I don't see
any way in which it could be worse compared to what we have now.
Another example in which it would be useful to have threads that don't have
MessageLoop is Pepper plugins. Pepper requires that plugins that need to use
Pepper APIs on any thread other than main run PPB_MessageLoop on that thread.
Obviously you can't run PPB_MessageLoop and MessageLoop thread, but it is
possible to create TaskRunner that uses PPB_MessageLoop.

Yes, we could add some specific API's to inject task runner to timer, but then 
 (1) the Timer would be uglier,
 (2) the code would still depend on MessageLoop, even when you are not using
MessageLoop, which means MessageLoop code will be pulled into the plugin binary,
even when you don't need it.

On 2012/04/25 07:08:39, akalin wrote:
> +Ami, since we talked about TaskRunner and ::current()-like functions
> before.
> 
> On Wed, Apr 25, 2012 at 10:32 AM, William Chan (陈智昌)
> <willchan@chromium.org>wrote:
> 
> >
> >> If MessageLoopProxy is not meant to be an interface then why does it need
> >> to be separate from MessageLoopProxyImpl?
> >>
> >
> > Very good question! I've wondered that often myself :)
> >
> > I honestly don't see why BrowserThread does this either. I took a glance
> > and it looks to me like we could just use MessageLoopProxyImpl everywhere.
> > I think this is a bug, unless we're doing this for testing reasons, which
> > may have been a good reason before, but now that we have TaskRunners,
> > there's no reason to do so now. I just asked jam@ and he agrees with me,
> > so we're going to kill off BrowserThread's subclass. I'm filing a bug.
> >
> 
> It's just an artifact of how things used to be before TaskRunners.  It's
> okay to combine MLP and MLPI now that we have SingleThreadTaskRunner.
> 
> 
> >
> >> Would it make sense to add current() in SingleThreadTaskRunner()?
> >>
> >
> > That's a good question. I am leaning towards saying that's OK by me. Lemme
> > think it over. We would probably want to make SingleThreadTaskRunner
> > set/unset a ThreadLocalPointer in its constructor/destructor, in a scoped
> > fashion, since they may be nested.
> >
> 
> I don't like this, actually. :/ If we're combining MLP and MLPI, then we
> can still have MLP::current() (since it's no worse than
> MessageLoop::current()) but I don't want to introduce a new global-like
> object, since it introduces weird action-at-a-distance effects, especially
> with reentrancy and possible future implementations of
> SingleThreadTaskRunner.  I'd prefer adding a method to base::Timer() or
> something, if possible.
> 
> I'm on vacation until next week or so, so maybe we can talk about it more
> then, unless you want this CL to go in soon.  Ami may have some thoughts
> here, also, since he's familiar with similar problems on the google3 side.

[Sergey Ulanov, 2012-04-25 17:47:26]

On 2012/04/25 17:27:55, Ami Fischman wrote:
> drive-by thoughts; feel free to ignore.
> 
> My reaction to this CL is to wonder why put current() in
SingleThreadTaskRunner
> and not further up the hierarchy, the logical conclusion of which is
TaskRunner;
> is there a principled reason STTR deserves current() more than TR?  

TaskRunner is more abstract than SignleThreadTaskRunner. E.g. TaskRunner may use
multiple threads to run it's task. Because of that the concept of the "current"
TaskRunner becomes very fuzzy. If multiple tasks runners share some threads,
what's the current TaskRunner for these threads?
In the same time it's easy to define what SingleThreadTaskRunner::current() is
supposed to return - the task runner that corresponds to the current thread.

Would it be better if instead of SingleThreadTaskRunner::current() we call it
SingleThreadTaskRunner::GetForCurrentThread()?

> OTOH, having a TaskRunner::current() would encourage just the kind of
> overly-optimistic programming errors I was worried about last time I spoke w/
> Fred about current(); namely that different layers of library code and
> application code all make assumptions about what they can do with a "current"
> TaskRunner, including deadlocking it ("surely there's another thread available
> to process my work while I wait for it to finish") and being surprised by
> concurrency ("surely this queued work will only kick off once I return from my
> current function").

I'm not suggesting to add TaskRunner::current() - that would be wrong, I agree
with you.


> 
> From the reviewlog history here & chatting with Sergey, there seem to be two
> goals to this CL:
> 1) Enable use of Timer on a thread that's not running MessageLoop.  This can
be
> more simply achieved by giving Timer::Start() a new param of a TaskRunner, and
> avoid its use of MessageLoop::current() except in a forwarding version of
> Start().

It would also require modifying all code that we need to run in the plugin that
currently uses timers (e.g. we need to use ProxyConfigServiceLinux in NPAPI
plugin).

> 2) Allowing plugins to avoid having to maintain their own pointers to the
> browser-provided task-running API, by letting them ask for a global "current"
> runner.  Having STTR::current() is only useful for that if something in the
> plugin calls SetCurrent(), but in that case why put that in STTR instead of in
> the plugin's code?

But that doesn't solve the problem with timers and other shared chromium code
that depends on MessageLoop::current().

> 
>
http://chromiumcodereview.appspot.com/10210008/diff/8001/base/single_thread_t...
> File base/single_thread_task_runner.cc (right):
> 
>
http://chromiumcodereview.appspot.com/10210008/diff/8001/base/single_thread_t...
> base/single_thread_task_runner.cc:21: return lazy_tls_ptr.Pointer()->Get();
> Can this DCHECK the ret value (see the TODO(darin) in MessageLoop::current()'s
> impl).
> 
>
http://chromiumcodereview.appspot.com/10210008/diff/8001/base/single_thread_t...
> base/single_thread_task_runner.cc:26: DCHECK(!current() || !new_current);
> WDYT of xor instead of !||! ?
> DCHECK(current() ^ new_current);

[Ami GONE FROM CHROMIUM, 2012-04-25 18:17:41]

>
> TaskRunner is more abstract than SignleThreadTaskRunner. E.g. TaskRunner
> may use
> multiple threads to run it's task. Because of that the concept of the
> "current"
> TaskRunner becomes very fuzzy. If multiple tasks runners share some
> threads,
> what's the current TaskRunner for these threads?
> In the same time it's easy to define what SingleThreadTaskRunner::**current()
> is
> supposed to return - the task runner that corresponds to the current
> thread.
>

That definition assumes that there is a single runner corresponding to the
current thread.  Example problems with this assumption include:
- In the case of a threadpool runner, the runner corresponding to a thread
is not a STTR.  What then? (NULL?  do you require callers to first verify
that there *is* an STTR around?)
- Multiple STTRs can share a single thread for running their tasks; which
STTR is the "current" one on that thread?
and so on.

Would it be better if instead of SingleThreadTaskRunner::**current() we
> call it
> SingleThreadTaskRunner::**GetForCurrentThread()?


I don't think so.


> OTOH, having a TaskRunner::current() would encourage just the kind of
>> overly-optimistic programming errors I was worried about last time I
>> spoke w/
>> Fred about current(); namely that different layers of library code and
>> application code all make assumptions about what they can do with a
>> "current"
>> TaskRunner, including deadlocking it ("surely there's another thread
>> available
>> to process my work while I wait for it to finish") and being surprised by
>> concurrency ("surely this queued work will only kick off once I return
>> from my
>> current function").
>>
> I'm not suggesting to add TaskRunner::current() - that would be wrong, I
> agree
> with you.


My real point was not just that TR::c() would be a mistake, but that
STTR:c() is a mistake for similar reasons.
The API involves an implicit assumption that isn't a good one to make.


>  From the reviewlog history here & chatting with Sergey, there seem to be
>> two
>> goals to this CL:
>> 1) Enable use of Timer on a thread that's not running MessageLoop.  This
>> can be
>
>  more simply achieved by giving Timer::Start() a new param of a
>> TaskRunner, and
>> avoid its use of MessageLoop::current() except in a forwarding version of
>> Start().
>
> It would also require modifying all code that we need to run in the plugin
> that
> currently uses timers (e.g. we need to use ProxyConfigServiceLinux in NPAPI
> plugin).


Right.


> 2) Allowing plugins to avoid having to maintain their own pointers to the
>> browser-provided task-running API, by letting them ask for a global
>> "current"
>> runner.  Having STTR::current() is only useful for that if something in
>> the
>> plugin calls SetCurrent(), but in that case why put that in STTR instead
>> of in
>> the plugin's code?
>
> But that doesn't solve the problem with timers and other shared chromium
> code
> that depends on MessageLoop::current().


Sure, that's #1 above.  I mentioned #2 only because you brought it up as a
bolstering reason for this CL in IM (and I'm pointing out that if this CL
couldn't stand on its own without #2, #2 shouldn't be a reason to do it
anyway).

[willchan no longer on Chromium, 2012-04-25 18:42:29]

So there seem to be a discussion of the following options now:
1) Add a current() method to
  a) TaskRunner or
  b) SingleThreadTaskRunner
2) Add a SingleThreadTaskRunner argument to Timer.

1a - As you guys have noted, this can lead to problems due to ambiguity of
which TaskRunner you're referring to, when we have nesting of TaskRunners.
I think this is bad and should be avoided.
1b - STTR::current() would add a new global. I don't think this is
problematic. Existence of globals is not a problem (ignoring the static
initializers/destructors), it's the *use* that is the problem. I don't
think adding STTR::current() will encourage people to use globals any more
than they already do with MessageLoop::current(). Although, as noted below,
STTR::current() has similar problems as TaskRunner::current().
2 - Timer is not thread safe. Member functions and the destructor must be
called on the same thread. Therefore, we cannot pass a TaskRunner into it.
Passing a SingleThreadTaskRunner could be acceptable, although we'd have to
validate that it's actually the SingleThreadTaskRunner that we're running
tasks on (RunsTasksOnCurrentThread()).

On Wed, Apr 25, 2012 at 11:17 AM, Ami Fischman <fischman@chromium.org>wrote:

> TaskRunner is more abstract than SignleThreadTaskRunner. E.g. TaskRunner
>> may use
>> multiple threads to run it's task. Because of that the concept of the
>> "current"
>> TaskRunner becomes very fuzzy. If multiple tasks runners share some
>> threads,
>> what's the current TaskRunner for these threads?
>> In the same time it's easy to define what
SingleThreadTaskRunner::**current()
>> is
>> supposed to return - the task runner that corresponds to the current
>> thread.
>>
>
> That definition assumes that there is a single runner corresponding to the
> current thread.  Example problems with this assumption include:
> - In the case of a threadpool runner, the runner corresponding to a thread
> is not a STTR.  What then? (NULL?  do you require callers to first verify
> that there *is* an STTR around?)
>

In most code where you would use current(), you *know* you're running on a
STTR. That's why we don't check MessageLoop::current().


> - Multiple STTRs can share a single thread for running their tasks; which
> STTR is the "current" one on that thread?
> and so on.
>

In my mind, current() would be the most narrowly scoped STTR. Even if
multiple STTR share the same thread, current() would refer to the one we're
actively running a task for. In the case of nesting, the most narrow scope
applies.

That said, I agree that in the nesting case, we could run into problems
where app code X calls into library Y which nests a STTR. Library Y calls
back into X via a Delegate interface. Now using app code X in
STTR::current() would cause problems. It may be the case we need a tighter
restriction on STTR, that you cannot nest. Indeed, you cannot nest multiple
MessageLoops, although you can nest Run() calls in specific cases. I
suspect we probably want to convert STTR into a
SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then only
expose a current() on STETR.


>
> Would it be better if instead of SingleThreadTaskRunner::**current() we
>> call it
>> SingleThreadTaskRunner::**GetForCurrentThread()?
>
>
> I don't think so.
>
>
>> OTOH, having a TaskRunner::current() would encourage just the kind of
>>> overly-optimistic programming errors I was worried about last time I
>>> spoke w/
>>> Fred about current(); namely that different layers of library code and
>>> application code all make assumptions about what they can do with a
>>> "current"
>>> TaskRunner, including deadlocking it ("surely there's another thread
>>> available
>>> to process my work while I wait for it to finish") and being surprised by
>>> concurrency ("surely this queued work will only kick off once I return
>>> from my
>>> current function").
>>>
>> I'm not suggesting to add TaskRunner::current() - that would be wrong, I
>> agree
>> with you.
>
>
> My real point was not just that TR::c() would be a mistake, but that
> STTR:c() is a mistake for similar reasons.
> The API involves an implicit assumption that isn't a good one to make.
>
>
>>  From the reviewlog history here & chatting with Sergey, there seem to be
>>> two
>>> goals to this CL:
>>> 1) Enable use of Timer on a thread that's not running MessageLoop.  This
>>> can be
>>
>>  more simply achieved by giving Timer::Start() a new param of a
>>> TaskRunner, and
>>> avoid its use of MessageLoop::current() except in a forwarding version of
>>> Start().
>>
>> It would also require modifying all code that we need to run in the
>> plugin that
>> currently uses timers (e.g. we need to use ProxyConfigServiceLinux in
>> NPAPI
>> plugin).
>
>
> Right.
>
>
>> 2) Allowing plugins to avoid having to maintain their own pointers to the
>>> browser-provided task-running API, by letting them ask for a global
>>> "current"
>>> runner.  Having STTR::current() is only useful for that if something in
>>> the
>>> plugin calls SetCurrent(), but in that case why put that in STTR instead
>>> of in
>>> the plugin's code?
>>
>> But that doesn't solve the problem with timers and other shared chromium
>> code
>> that depends on MessageLoop::current().
>
>
> Sure, that's #1 above.  I mentioned #2 only because you brought it up as a
> bolstering reason for this CL in IM (and I'm pointing out that if this CL
> couldn't stand on its own without #2, #2 shouldn't be a reason to do it
> anyway).
>
>

[Ami GONE FROM CHROMIUM, 2012-04-25 19:27:30]

On Wed, Apr 25, 2012 at 11:42 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> So there seem to be a discussion of the following options now:
> 1) Add a current() method to
>   a) TaskRunner or
>   b) SingleThreadTaskRunner
> 2) Add a SingleThreadTaskRunner argument to Timer.
>
> 1a - As you guys have noted, this can lead to problems due to ambiguity of
> which TaskRunner you're referring to, when we have nesting of TaskRunners.
> I think this is bad and should be avoided.
> 1b - STTR::current() would add a new global. I don't think this is
> problematic. Existence of globals is not a problem (ignoring the static
> initializers/destructors), it's the *use* that is the problem. I don't
> think adding STTR::current() will encourage people to use globals any more
> than they already do with MessageLoop::current(). Although, as noted below,
> STTR::current() has similar problems as TaskRunner::current().
> 2 - Timer is not thread safe. Member functions and the destructor must be
> called on the same thread. Therefore, we cannot pass a TaskRunner into it.
> Passing a SingleThreadTaskRunner could be acceptable, although we'd have to
> validate that it's actually the SingleThreadTaskRunner that we're running
> tasks on (RunsTasksOnCurrentThread()).
>

Or, of course, Timer could be made thread-safe ;)


> - In the case of a threadpool runner, the runner corresponding to a thread
>> is not a STTR.  What then? (NULL?  do you require callers to first verify
>> that there *is* an STTR around?)
>>
> In most code where you would use current(), you *know* you're running on a
> STTR. That's why we don't check MessageLoop::current().
>

s/*know*/*assume*/
Most chromium library code feels free to call MessageLoop::current()
because the use of thread-pool-based runners is very new and most chromium
devs don't spend time thinking about that possibility.

- Multiple STTRs can share a single thread for running their tasks; which
>> STTR is the "current" one on that thread?
>> and so on.
>>
> In my mind, current() would be the most narrowly scoped STTR. Even if
> multiple STTR share the same thread, current() would refer to the one we're
> actively running a task for. In the case of nesting, the most narrow scope
> applies.
>

To be clear, nesting isn't the only way to share.  I think you're saying
STTR::c() should return the STTR that was used to post the current task to
the thread.
That is well-defined (once you assert that *some* STTR was involved in
posting the current stack), but would require changes to carry this
information along with the task and set/unset the relevant state around
execution.


> That said, I agree that in the nesting case, we could run into problems
> where app code X calls into library Y which nests a STTR. Library Y calls
> back into X via a Delegate interface. Now using app code X in
> STTR::current() would cause problems. It may be the case we need a tighter
> restriction on STTR, that you cannot nest. Indeed, you cannot nest multiple
> MessageLoops, although you can nest Run() calls in specific cases. I
> suspect we probably want to convert STTR into a
> SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then only
> expose a current() on STETR.
>

If you wanted to use thread identity to implement STETR::c() then STETR
would also have to impose a restriction on the thread that it used for
execution that it was used exclusively by that STETR.  IOW, you'd have to
forbid the case that STTR today allows, where two (or more) STTRs can be
PostTask'ing to the same ML-based thread for execution.  That seems to be
leaking an impl detail (the desire to be able to use TLS to implement
current()) into the API of the STETR interface, so a bad thing.  So
probably it'd need to go the route of packaging up the STETR's identity
into the posted tasks so they could be set/unset around execution.

[Sergey Ulanov, 2012-04-25 19:38:52]

I agree that the situation when there are multiple STTR instances per
thread may be problematic and we should be careful about it. I actually
think there aren't that many cases when multiple STTRs per thread are
useful. But when we do need to nest STTRs I think current() should return
the root one (or maybe even NULL). In fact, there is already DCHECK() in
SetCurrent() that prevents current() from being changed when it is already
set. We actually may stipulate in the comments that STTR implementations
must not call SetCurrent(). Instead SetCurrent() must be invoked by the
code responsible for creation of that thread and the root task runner.
For threads that are part of a thread pool I believe that STTR::current()
should always return NULL - this will also have side effect of preventing
usage of timers on thread pools, which is a bad idea anyway.

On Wed, Apr 25, 2012 at 11:42 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> So there seem to be a discussion of the following options now:
> 1) Add a current() method to
>   a) TaskRunner or
>   b) SingleThreadTaskRunner
> 2) Add a SingleThreadTaskRunner argument to Timer.
>
> 1a - As you guys have noted, this can lead to problems due to ambiguity of
> which TaskRunner you're referring to, when we have nesting of TaskRunners.
> I think this is bad and should be avoided.
> 1b - STTR::current() would add a new global. I don't think this is
> problematic. Existence of globals is not a problem (ignoring the static
> initializers/destructors), it's the *use* that is the problem. I don't
> think adding STTR::current() will encourage people to use globals any more
> than they already do with MessageLoop::current(). Although, as noted below,
> STTR::current() has similar problems as TaskRunner::current().
> 2 - Timer is not thread safe. Member functions and the destructor must be
> called on the same thread. Therefore, we cannot pass a TaskRunner into it.
> Passing a SingleThreadTaskRunner could be acceptable, although we'd have to
> validate that it's actually the SingleThreadTaskRunner that we're running
> tasks on (RunsTasksOnCurrentThread()).
>
> On Wed, Apr 25, 2012 at 11:17 AM, Ami Fischman <fischman@chromium.org>wrote:
>
>> TaskRunner is more abstract than SignleThreadTaskRunner. E.g. TaskRunner
>>> may use
>>> multiple threads to run it's task. Because of that the concept of the
>>> "current"
>>> TaskRunner becomes very fuzzy. If multiple tasks runners share some
>>> threads,
>>> what's the current TaskRunner for these threads?
>>> In the same time it's easy to define what
SingleThreadTaskRunner::**current()
>>> is
>>> supposed to return - the task runner that corresponds to the current
>>> thread.
>>>
>>
>> That definition assumes that there is a single runner corresponding to
>> the current thread.  Example problems with this assumption include:
>> - In the case of a threadpool runner, the runner corresponding to a
>> thread is not a STTR.  What then? (NULL?  do you require callers to first
>> verify that there *is* an STTR around?)
>>
>
> In most code where you would use current(), you *know* you're running on a
> STTR. That's why we don't check MessageLoop::current().
>
>
>> - Multiple STTRs can share a single thread for running their tasks; which
>> STTR is the "current" one on that thread?
>> and so on.
>>
>
> In my mind, current() would be the most narrowly scoped STTR. Even if
> multiple STTR share the same thread, current() would refer to the one we're
> actively running a task for. In the case of nesting, the most narrow scope
> applies.
>
> That said, I agree that in the nesting case, we could run into problems
> where app code X calls into library Y which nests a STTR. Library Y calls
> back into X via a Delegate interface. Now using app code X in
> STTR::current() would cause problems. It may be the case we need a tighter
> restriction on STTR, that you cannot nest. Indeed, you cannot nest multiple
> MessageLoops, although you can nest Run() calls in specific cases. I
> suspect we probably want to convert STTR into a
> SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then only
> expose a current() on STETR.
>

This looks like a good idea. Maybe call it ThreadTaskRunner instead
of SingleThreadExclusiveTaskRunner? SingleThreadExclusiveTaskRunner seems
to be too long.


>
>
>>
>> Would it be better if instead of SingleThreadTaskRunner::**current() we
>>> call it
>>> SingleThreadTaskRunner::**GetForCurrentThread()?
>>
>>
>> I don't think so.
>>
>>
>>> OTOH, having a TaskRunner::current() would encourage just the kind of
>>>> overly-optimistic programming errors I was worried about last time I
>>>> spoke w/
>>>> Fred about current(); namely that different layers of library code and
>>>> application code all make assumptions about what they can do with a
>>>> "current"
>>>> TaskRunner, including deadlocking it ("surely there's another thread
>>>> available
>>>> to process my work while I wait for it to finish") and being surprised
>>>> by
>>>> concurrency ("surely this queued work will only kick off once I return
>>>> from my
>>>> current function").
>>>>
>>> I'm not suggesting to add TaskRunner::current() - that would be wrong, I
>>> agree
>>> with you.
>>
>>
>> My real point was not just that TR::c() would be a mistake, but that
>> STTR:c() is a mistake for similar reasons.
>> The API involves an implicit assumption that isn't a good one to make.
>>
>>
>>>  From the reviewlog history here & chatting with Sergey, there seem to
>>>> be two
>>>> goals to this CL:
>>>> 1) Enable use of Timer on a thread that's not running MessageLoop.
>>>>  This can be
>>>
>>>  more simply achieved by giving Timer::Start() a new param of a
>>>> TaskRunner, and
>>>> avoid its use of MessageLoop::current() except in a forwarding version
>>>> of
>>>> Start().
>>>
>>> It would also require modifying all code that we need to run in the
>>> plugin that
>>> currently uses timers (e.g. we need to use ProxyConfigServiceLinux in
>>> NPAPI
>>> plugin).
>>
>>
>> Right.
>>
>>
>>> 2) Allowing plugins to avoid having to maintain their own pointers to the
>>>> browser-provided task-running API, by letting them ask for a global
>>>> "current"
>>>> runner.  Having STTR::current() is only useful for that if something in
>>>> the
>>>> plugin calls SetCurrent(), but in that case why put that in STTR
>>>> instead of in
>>>> the plugin's code?
>>>
>>> But that doesn't solve the problem with timers and other shared chromium
>>> code
>>> that depends on MessageLoop::current().
>>
>>
>> Sure, that's #1 above.  I mentioned #2 only because you brought it up as
>> a bolstering reason for this CL in IM (and I'm pointing out that if this CL
>> couldn't stand on its own without #2, #2 shouldn't be a reason to do it
>> anyway).
>>
>>
>

[willchan no longer on Chromium, 2012-04-25 20:20:22]

On Wed, Apr 25, 2012 at 12:27 PM, Ami Fischman <fischman@chromium.org>wrote:

> On Wed, Apr 25, 2012 at 11:42 AM, William Chan (陈智昌) <
> willchan@chromium.org> wrote:
>
>> So there seem to be a discussion of the following options now:
>> 1) Add a current() method to
>>   a) TaskRunner or
>>   b) SingleThreadTaskRunner
>> 2) Add a SingleThreadTaskRunner argument to Timer.
>>
>> 1a - As you guys have noted, this can lead to problems due to ambiguity
>> of which TaskRunner you're referring to, when we have nesting of
>> TaskRunners. I think this is bad and should be avoided.
>> 1b - STTR::current() would add a new global. I don't think this is
>> problematic. Existence of globals is not a problem (ignoring the static
>> initializers/destructors), it's the *use* that is the problem. I don't
>> think adding STTR::current() will encourage people to use globals any more
>> than they already do with MessageLoop::current(). Although, as noted below,
>> STTR::current() has similar problems as TaskRunner::current().
>> 2 - Timer is not thread safe. Member functions and the destructor must be
>> called on the same thread. Therefore, we cannot pass a TaskRunner into it.
>> Passing a SingleThreadTaskRunner could be acceptable, although we'd have to
>> validate that it's actually the SingleThreadTaskRunner that we're running
>> tasks on (RunsTasksOnCurrentThread()).
>>
>
> Or, of course, Timer could be made thread-safe ;)
>

I don't believe this is true. I believe trying to revoke (Timer::Reset()) a
timer from a different thread than the one it's currently running on is
fundamentally racy, since the task may be concurrently running.


>
>
>> - In the case of a threadpool runner, the runner corresponding to a
>>> thread is not a STTR.  What then? (NULL?  do you require callers to first
>>> verify that there *is* an STTR around?)
>>>
>> In most code where you would use current(), you *know* you're running on
>> a STTR. That's why we don't check MessageLoop::current().
>>
>
> s/*know*/*assume*/
>

Sorry, yes.


> Most chromium library code feels free to call MessageLoop::current()
> because the use of thread-pool-based runners is very new and most chromium
> devs don't spend time thinking about that possibility.
>

WorkerPools have existed for awhile...but yes, we don't normally use them.


>
> - Multiple STTRs can share a single thread for running their tasks; which
>>> STTR is the "current" one on that thread?
>>> and so on.
>>>
>> In my mind, current() would be the most narrowly scoped STTR. Even if
>> multiple STTR share the same thread, current() would refer to the one we're
>> actively running a task for. In the case of nesting, the most narrow scope
>> applies.
>>
>
> To be clear, nesting isn't the only way to share.  I think you're saying
> STTR::c() should return the STTR that was used to post the current task to
> the thread.
> That is well-defined (once you assert that *some* STTR was involved in
> posting the current stack), but would require changes to carry this
> information along with the task and set/unset the relevant state around
> execution.
>

Yes. And I don't believe that's a big hurdle. And I believe we can write
tests to assert STTR implementations implement it correctly.


>
>
>> That said, I agree that in the nesting case, we could run into problems
>> where app code X calls into library Y which nests a STTR. Library Y calls
>> back into X via a Delegate interface. Now using app code X in
>> STTR::current() would cause problems. It may be the case we need a tighter
>> restriction on STTR, that you cannot nest. Indeed, you cannot nest multiple
>> MessageLoops, although you can nest Run() calls in specific cases. I
>> suspect we probably want to convert STTR into a
>> SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then only
>> expose a current() on STETR.
>>
>
> If you wanted to use thread identity to implement STETR::c() then STETR
> would also have to impose a restriction on the thread that it used for
> execution that it was used exclusively by that STETR.  IOW, you'd have to
> forbid the case that STTR today allows, where two (or more) STTRs can be
> PostTask'ing to the same ML-based thread for execution.  That seems to be
> leaking an impl detail (the desire to be able to use TLS to implement
> current()) into the API of the STETR interface, so a bad thing.  So
> probably it'd need to go the route of packaging up the STETR's identity
> into the posted tasks so they could be set/unset around execution.
>
>
For your first sentence here, yes, that's what I meant by the E
(exclusive). Your second point is very good, and I forgot about that. I'm
going to go think about it while I get lunch :) Let me say that my point in
bringing up new restrictions is that I don't want us to get too caught up
in abstract perfection. I'm fine with restricting the flexibility of
certain subclasses to make them feasible for solving our current problem,
since I don't believe it makes our world worse than it already is.

[willchan no longer on Chromium, 2012-04-25 20:30:03]

I need to run to a meeting, but my new inclination is that base::Timer may
be fundamentally tied to MessageLoop. It may simply be better to use a
different paradigm from the ProxyConfigServiceLinux code. Need to think
about it more, but meeting time. I'll let you guys mull that thought over.

On Wed, Apr 25, 2012 at 1:20 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Apr 25, 2012 at 12:27 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>> On Wed, Apr 25, 2012 at 11:42 AM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> So there seem to be a discussion of the following options now:
>>>  1) Add a current() method to
>>>   a) TaskRunner or
>>>   b) SingleThreadTaskRunner
>>> 2) Add a SingleThreadTaskRunner argument to Timer.
>>>
>>> 1a - As you guys have noted, this can lead to problems due to ambiguity
>>> of which TaskRunner you're referring to, when we have nesting of
>>> TaskRunners. I think this is bad and should be avoided.
>>> 1b - STTR::current() would add a new global. I don't think this is
>>> problematic. Existence of globals is not a problem (ignoring the static
>>> initializers/destructors), it's the *use* that is the problem. I don't
>>> think adding STTR::current() will encourage people to use globals any more
>>> than they already do with MessageLoop::current(). Although, as noted below,
>>> STTR::current() has similar problems as TaskRunner::current().
>>> 2 - Timer is not thread safe. Member functions and the destructor must
>>> be called on the same thread. Therefore, we cannot pass a TaskRunner into
>>> it. Passing a SingleThreadTaskRunner could be acceptable, although we'd
>>> have to validate that it's actually the SingleThreadTaskRunner that we're
>>> running tasks on (RunsTasksOnCurrentThread()).
>>>
>>
>> Or, of course, Timer could be made thread-safe ;)
>>
>
> I don't believe this is true. I believe trying to revoke (Timer::Reset())
> a timer from a different thread than the one it's currently running on is
> fundamentally racy, since the task may be concurrently running.
>
>
>>
>>
>>>  - In the case of a threadpool runner, the runner corresponding to a
>>>> thread is not a STTR.  What then? (NULL?  do you require callers to first
>>>> verify that there *is* an STTR around?)
>>>>
>>> In most code where you would use current(), you *know* you're running on
>>> a STTR. That's why we don't check MessageLoop::current().
>>>
>>
>> s/*know*/*assume*/
>>
>
> Sorry, yes.
>
>
>> Most chromium library code feels free to call MessageLoop::current()
>> because the use of thread-pool-based runners is very new and most chromium
>> devs don't spend time thinking about that possibility.
>>
>
> WorkerPools have existed for awhile...but yes, we don't normally use them.
>
>
>>
>> - Multiple STTRs can share a single thread for running their tasks; which
>>>> STTR is the "current" one on that thread?
>>>> and so on.
>>>>
>>> In my mind, current() would be the most narrowly scoped STTR. Even if
>>> multiple STTR share the same thread, current() would refer to the one we're
>>> actively running a task for. In the case of nesting, the most narrow scope
>>> applies.
>>>
>>
>> To be clear, nesting isn't the only way to share.  I think you're saying
>> STTR::c() should return the STTR that was used to post the current task to
>> the thread.
>>  That is well-defined (once you assert that *some* STTR was involved in
>> posting the current stack), but would require changes to carry this
>> information along with the task and set/unset the relevant state around
>> execution.
>>
>
> Yes. And I don't believe that's a big hurdle. And I believe we can write
> tests to assert STTR implementations implement it correctly.
>
>
>>
>>
>>> That said, I agree that in the nesting case, we could run into problems
>>> where app code X calls into library Y which nests a STTR. Library Y calls
>>> back into X via a Delegate interface. Now using app code X in
>>> STTR::current() would cause problems. It may be the case we need a tighter
>>> restriction on STTR, that you cannot nest. Indeed, you cannot nest multiple
>>> MessageLoops, although you can nest Run() calls in specific cases. I
>>> suspect we probably want to convert STTR into a
>>> SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then only
>>> expose a current() on STETR.
>>>
>>
>> If you wanted to use thread identity to implement STETR::c() then STETR
>> would also have to impose a restriction on the thread that it used for
>> execution that it was used exclusively by that STETR.  IOW, you'd have to
>> forbid the case that STTR today allows, where two (or more) STTRs can be
>> PostTask'ing to the same ML-based thread for execution.  That seems to be
>> leaking an impl detail (the desire to be able to use TLS to implement
>> current()) into the API of the STETR interface, so a bad thing.  So
>> probably it'd need to go the route of packaging up the STETR's identity
>> into the posted tasks so they could be set/unset around execution.
>>
>>
> For your first sentence here, yes, that's what I meant by the E
> (exclusive). Your second point is very good, and I forgot about that. I'm
> going to go think about it while I get lunch :) Let me say that my point in
> bringing up new restrictions is that I don't want us to get too caught up
> in abstract perfection. I'm fine with restricting the flexibility of
> certain subclasses to make them feasible for solving our current problem,
> since I don't believe it makes our world worse than it already is.
>

[Sergey Ulanov, 2012-04-25 23:43:15]

On Wed, Apr 25, 2012 at 1:20 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Apr 25, 2012 at 12:27 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>> On Wed, Apr 25, 2012 at 11:42 AM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> So there seem to be a discussion of the following options now:
>>>  1) Add a current() method to
>>>   a) TaskRunner or
>>>   b) SingleThreadTaskRunner
>>> 2) Add a SingleThreadTaskRunner argument to Timer.
>>>
>>> 1a - As you guys have noted, this can lead to problems due to ambiguity
>>> of which TaskRunner you're referring to, when we have nesting of
>>> TaskRunners. I think this is bad and should be avoided.
>>> 1b - STTR::current() would add a new global. I don't think this is
>>> problematic. Existence of globals is not a problem (ignoring the static
>>> initializers/destructors), it's the *use* that is the problem. I don't
>>> think adding STTR::current() will encourage people to use globals any more
>>> than they already do with MessageLoop::current(). Although, as noted below,
>>> STTR::current() has similar problems as TaskRunner::current().
>>> 2 - Timer is not thread safe. Member functions and the destructor must
>>> be called on the same thread. Therefore, we cannot pass a TaskRunner into
>>> it. Passing a SingleThreadTaskRunner could be acceptable, although we'd
>>> have to validate that it's actually the SingleThreadTaskRunner that we're
>>> running tasks on (RunsTasksOnCurrentThread()).
>>>
>>
>> Or, of course, Timer could be made thread-safe ;)
>>
>
> I don't believe this is true. I believe trying to revoke (Timer::Reset())
> a timer from a different thread than the one it's currently running on is
> fundamentally racy, since the task may be concurrently running.
>
>
>>
>>
>>>  - In the case of a threadpool runner, the runner corresponding to a
>>>> thread is not a STTR.  What then? (NULL?  do you require callers to first
>>>> verify that there *is* an STTR around?)
>>>>
>>> In most code where you would use current(), you *know* you're running on
>>> a STTR. That's why we don't check MessageLoop::current().
>>>
>>
>> s/*know*/*assume*/
>>
>
> Sorry, yes.
>
>
>> Most chromium library code feels free to call MessageLoop::current()
>> because the use of thread-pool-based runners is very new and most chromium
>> devs don't spend time thinking about that possibility.
>>
>
> WorkerPools have existed for awhile...but yes, we don't normally use them.
>
>
>>
>> - Multiple STTRs can share a single thread for running their tasks; which
>>>> STTR is the "current" one on that thread?
>>>> and so on.
>>>>
>>> In my mind, current() would be the most narrowly scoped STTR. Even if
>>> multiple STTR share the same thread, current() would refer to the one we're
>>> actively running a task for. In the case of nesting, the most narrow scope
>>> applies.
>>>
>>
>> To be clear, nesting isn't the only way to share.  I think you're saying
>> STTR::c() should return the STTR that was used to post the current task to
>> the thread.
>>  That is well-defined (once you assert that *some* STTR was involved in
>> posting the current stack), but would require changes to carry this
>> information along with the task and set/unset the relevant state around
>> execution.
>>
>
> Yes. And I don't believe that's a big hurdle. And I believe we can write
> tests to assert STTR implementations implement it correctly.
>

I actually don't think allowing multiple STTR per thread and ensuring that
STTR::current() always returns the STTR that invoked the current task is a
good idea. Any examples when this could be useful?

What I suggest (as discribed in my previous reply) is a much simple
solution: we could have a single "root" STTR per thread and current() would
return it. There may be other layered STTRs but current() would never
return them. That would be easier to implement and IMHO less error prone.


>
>
>>
>>
>>> That said, I agree that in the nesting case, we could run into problems
>>> where app code X calls into library Y which nests a STTR. Library Y calls
>>> back into X via a Delegate interface. Now using app code X in
>>> STTR::current() would cause problems. It may be the case we need a tighter
>>> restriction on STTR, that you cannot nest. Indeed, you cannot nest multiple
>>> MessageLoops, although you can nest Run() calls in specific cases. I
>>> suspect we probably want to convert STTR into a
>>> SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then only
>>> expose a current() on STETR.
>>>
>>
>> If you wanted to use thread identity to implement STETR::c() then STETR
>> would also have to impose a restriction on the thread that it used for
>> execution that it was used exclusively by that STETR.  IOW, you'd have to
>> forbid the case that STTR today allows, where two (or more) STTRs can be
>> PostTask'ing to the same ML-based thread for execution.  That seems to be
>> leaking an impl detail (the desire to be able to use TLS to implement
>> current()) into the API of the STETR interface, so a bad thing.  So
>> probably it'd need to go the route of packaging up the STETR's identity
>> into the posted tasks so they could be set/unset around execution.
>>
>>
> For your first sentence here, yes, that's what I meant by the E
> (exclusive). Your second point is very good, and I forgot about that. I'm
> going to go think about it while I get lunch :) Let me say that my point in
> bringing up new restrictions is that I don't want us to get too caught up
> in abstract perfection. I'm fine with restricting the flexibility of
> certain subclasses to make them feasible for solving our current problem,
> since I don't believe it makes our world worse than it already is.
>

[Sergey Ulanov, 2012-04-25 23:46:55]

On Wed, Apr 25, 2012 at 1:30 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> I need to run to a meeting, but my new inclination is that base::Timer may
> be fundamentally tied to MessageLoop.
>

Why? Currently it just calls PostDelayedTask() for the MessageLoop returned
by MessageLoop::current(). It doesn't really care how the delayed task is
invoked as it is invoked on the same thread it was posted.



> It may simply be better to use a different paradigm from the
> ProxyConfigServiceLinux code. Need to think about it more, but meeting
> time. I'll let you guys mull that thought over.
>

>
>
>

[willchan no longer on Chromium, 2012-04-26 00:11:19]

On Wed, Apr 25, 2012 at 4:42 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

>
>
> On Wed, Apr 25, 2012 at 1:20 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Wed, Apr 25, 2012 at 12:27 PM, Ami Fischman <fischman@chromium.org>wrote:
>>
>>> On Wed, Apr 25, 2012 at 11:42 AM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> So there seem to be a discussion of the following options now:
>>>>  1) Add a current() method to
>>>>   a) TaskRunner or
>>>>   b) SingleThreadTaskRunner
>>>> 2) Add a SingleThreadTaskRunner argument to Timer.
>>>>
>>>> 1a - As you guys have noted, this can lead to problems due to ambiguity
>>>> of which TaskRunner you're referring to, when we have nesting of
>>>> TaskRunners. I think this is bad and should be avoided.
>>>> 1b - STTR::current() would add a new global. I don't think this is
>>>> problematic. Existence of globals is not a problem (ignoring the static
>>>> initializers/destructors), it's the *use* that is the problem. I don't
>>>> think adding STTR::current() will encourage people to use globals any more
>>>> than they already do with MessageLoop::current(). Although, as noted
below,
>>>> STTR::current() has similar problems as TaskRunner::current().
>>>> 2 - Timer is not thread safe. Member functions and the destructor must
>>>> be called on the same thread. Therefore, we cannot pass a TaskRunner into
>>>> it. Passing a SingleThreadTaskRunner could be acceptable, although we'd
>>>> have to validate that it's actually the SingleThreadTaskRunner that we're
>>>> running tasks on (RunsTasksOnCurrentThread()).
>>>>
>>>
>>> Or, of course, Timer could be made thread-safe ;)
>>>
>>
>> I don't believe this is true. I believe trying to revoke (Timer::Reset())
>> a timer from a different thread than the one it's currently running on is
>> fundamentally racy, since the task may be concurrently running.
>>
>>
>>>
>>>
>>>>  - In the case of a threadpool runner, the runner corresponding to a
>>>>> thread is not a STTR.  What then? (NULL?  do you require callers to first
>>>>> verify that there *is* an STTR around?)
>>>>>
>>>> In most code where you would use current(), you *know* you're running
>>>> on a STTR. That's why we don't check MessageLoop::current().
>>>>
>>>
>>> s/*know*/*assume*/
>>>
>>
>> Sorry, yes.
>>
>>
>>> Most chromium library code feels free to call MessageLoop::current()
>>> because the use of thread-pool-based runners is very new and most chromium
>>> devs don't spend time thinking about that possibility.
>>>
>>
>> WorkerPools have existed for awhile...but yes, we don't normally use them.
>>
>>
>>>
>>> - Multiple STTRs can share a single thread for running their tasks;
>>>>> which STTR is the "current" one on that thread?
>>>>> and so on.
>>>>>
>>>> In my mind, current() would be the most narrowly scoped STTR. Even if
>>>> multiple STTR share the same thread, current() would refer to the one
we're
>>>> actively running a task for. In the case of nesting, the most narrow scope
>>>> applies.
>>>>
>>>
>>> To be clear, nesting isn't the only way to share.  I think you're saying
>>> STTR::c() should return the STTR that was used to post the current task to
>>> the thread.
>>>  That is well-defined (once you assert that *some* STTR was involved in
>>> posting the current stack), but would require changes to carry this
>>> information along with the task and set/unset the relevant state around
>>> execution.
>>>
>>
>> Yes. And I don't believe that's a big hurdle. And I believe we can write
>> tests to assert STTR implementations implement it correctly.
>>
>
> I actually don't think allowing multiple STTR per thread and ensuring that
> STTR::current() always returns the STTR that invoked the current task is a
> good idea. Any examples when this could be useful?
>

I think we kinda sorta do something like this for testing. In unit_tests,
we often map multiple BrowserThread IDs to the same underlying MessageLoop.


>
> What I suggest (as discribed in my previous reply) is a much simple
> solution: we could have a single "root" STTR per thread and current() would
> return it. There may be other layered STTRs but current() would never
> return them. That would be easier to implement and IMHO less error prone.
>

It's not immediately obvious to me this is less error prone...let's say I
write library code where I don't know whether or not I'm operating within
the context of another STTR. I start up a STTR and use a base::Timer (which
we hypothetically have changed to use STTR::current()). Let's say we expect
the Timer to exit the STTR. But if the STTR was nested and thus not the
"root" (I guess it depends on your definition of root), then this Timer
will post a task to the "root" STTR and not the one we're currently running
in, and thus won't exit...


>
>
>>
>>
>>>
>>>
>>>> That said, I agree that in the nesting case, we could run into problems
>>>> where app code X calls into library Y which nests a STTR. Library Y calls
>>>> back into X via a Delegate interface. Now using app code X in
>>>> STTR::current() would cause problems. It may be the case we need a tighter
>>>> restriction on STTR, that you cannot nest. Indeed, you cannot nest
multiple
>>>> MessageLoops, although you can nest Run() calls in specific cases. I
>>>> suspect we probably want to convert STTR into a
>>>> SingleThreadExclusiveTaskRunner to prevent nesting of STETRs. And then
only
>>>> expose a current() on STETR.
>>>>
>>>
>>> If you wanted to use thread identity to implement STETR::c() then STETR
>>> would also have to impose a restriction on the thread that it used for
>>> execution that it was used exclusively by that STETR.  IOW, you'd have to
>>> forbid the case that STTR today allows, where two (or more) STTRs can be
>>> PostTask'ing to the same ML-based thread for execution.  That seems to be
>>> leaking an impl detail (the desire to be able to use TLS to implement
>>> current()) into the API of the STETR interface, so a bad thing.  So
>>> probably it'd need to go the route of packaging up the STETR's identity
>>> into the posted tasks so they could be set/unset around execution.
>>>
>>>
>> For your first sentence here, yes, that's what I meant by the E
>> (exclusive). Your second point is very good, and I forgot about that. I'm
>> going to go think about it while I get lunch :) Let me say that my point in
>> bringing up new restrictions is that I don't want us to get too caught up
>> in abstract perfection. I'm fine with restricting the flexibility of
>> certain subclasses to make them feasible for solving our current problem,
>> since I don't believe it makes our world worse than it already is.
>>
>
>

[willchan no longer on Chromium, 2012-04-26 00:14:39]

On Wed, Apr 25, 2012 at 4:46 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> On Wed, Apr 25, 2012 at 1:30 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> I need to run to a meeting, but my new inclination is that base::Timer
>> may be fundamentally tied to MessageLoop.
>>
>
> Why? Currently it just calls PostDelayedTask() for the MessageLoop
> returned by MessageLoop::current(). It doesn't really care how the delayed
> task is invoked as it is invoked on the same thread it was posted.
>

I guess my statement was overly strong. We'd have to have a TaskRunner
subtype that was very restrictive. It'd only allow one instance of that
type to exist on a given thread at any point, and would not allow any
others to be instantiated. This is basically what MessageLoop itself
requires (there's a DCHECK that MessageLoop::current() is NULL when
MessageLoop is ocnstructed). If we provide that guarantee, then this could
work. This is pretty close to what Ami was describing as implementation
leaking through the interface, unless we decide that the interface requires
that there only be a single instance on a given thread.


>
>
>
>> It may simply be better to use a different paradigm from the
>> ProxyConfigServiceLinux code. Need to think about it more, but meeting
>> time. I'll let you guys mull that thought over.
>>
>
>>
>>
>>

[Sergey Ulanov, 2012-04-26 00:32:56]

On Wed, Apr 25, 2012 at 5:11 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Apr 25, 2012 at 4:42 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>>
>> I actually don't think allowing multiple STTR per thread and ensuring
>> that STTR::current() always returns the STTR that invoked the current task
>> is a good idea. Any examples when this could be useful?
>>
>
> I think we kinda sorta do something like this for testing. In unit_tests,
> we often map multiple BrowserThread IDs to the same underlying MessageLoop.
>

But that's different. In that case there is still have just only one
MessageLoop. It's just that the tests lies to the the code being tested,
and passes that single MessageLoop pointer as if it was for a different
thread.


>
>
>>
>> What I suggest (as discribed in my previous reply) is a much simple
>> solution: we could have a single "root" STTR per thread and current() would
>> return it. There may be other layered STTRs but current() would never
>> return them. That would be easier to implement and IMHO less error prone.
>>
>
> It's not immediately obvious to me this is less error prone...let's say I
> write library code where I don't know whether or not I'm operating within
> the context of another STTR. I start up a STTR and use a base::Timer (which
> we hypothetically have changed to use STTR::current()). Let's say we expect
> the Timer to exit the STTR. But if the STTR was nested and thus not the
> "root" (I guess it depends on your definition of root), then this Timer
> will post a task to the "root" STTR and not the one we're currently running
> in, and thus won't exit...
>

I'm not sure I correctly understand the scenario you are describing. And we
may be talking about different things when we talk about nested thread
runner. If by nesting you mean invoking MessageLoop::Run() from a task that
was called form the main task runner (which is not a MessageLoop), then I
think that's a wrong approach anyway and should be avoided. Again, I don't
see any good valid scenario when it may be useful to have multiple thread
runners per one thread.

[Sergey Ulanov, 2012-04-26 00:38:16]

On Wed, Apr 25, 2012 at 5:14 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Apr 25, 2012 at 4:46 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>> On Wed, Apr 25, 2012 at 1:30 PM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> I need to run to a meeting, but my new inclination is that base::Timer
>>> may be fundamentally tied to MessageLoop.
>>>
>>
>> Why? Currently it just calls PostDelayedTask() for the MessageLoop
>> returned by MessageLoop::current(). It doesn't really care how the delayed
>> task is invoked as it is invoked on the same thread it was posted.
>>
>
> I guess my statement was overly strong. We'd have to have a TaskRunner
> subtype that was very restrictive. It'd only allow one instance of that
> type to exist on a given thread at any point, and would not allow any
> others to be instantiated. This is basically what MessageLoop itself
> requires (there's a DCHECK that MessageLoop::current() is NULL when
> MessageLoop is ocnstructed). If we provide that guarantee, then this could
> work.
>

There is already DCHECK in SetCurrent() in this CL that verifies that
SetCurrent() is not used to change current() when it is already set. I can
also add a DCHECK in the interface constructor to verify that current() is
set to NULL - do you think that would be enough?



> This is pretty close to what Ami was describing as implementation leaking
> through the interface, unless we decide that the interface requires that
> there only be a single instance on a given thread.
>
>
>>
>>
>>
>>> It may simply be better to use a different paradigm from the
>>> ProxyConfigServiceLinux code. Need to think about it more, but meeting
>>> time. I'll let you guys mull that thought over.
>>>
>>
>>>
>>>
>>>
>

[willchan no longer on Chromium, 2012-04-26 01:00:01]

On Wed, Apr 25, 2012 at 5:32 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

>
>
> On Wed, Apr 25, 2012 at 5:11 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Wed, Apr 25, 2012 at 4:42 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>>
>>>
>>> I actually don't think allowing multiple STTR per thread and ensuring
>>> that STTR::current() always returns the STTR that invoked the current task
>>> is a good idea. Any examples when this could be useful?
>>>
>>
>> I think we kinda sorta do something like this for testing. In unit_tests,
>> we often map multiple BrowserThread IDs to the same underlying MessageLoop.
>>
>
> But that's different. In that case there is still have just only one
> MessageLoop. It's just that the tests lies to the the code being tested,
> and passes that single MessageLoop pointer as if it was for a different
> thread.
>

>
>>
>>
>>>
>>> What I suggest (as discribed in my previous reply) is a much simple
>>> solution: we could have a single "root" STTR per thread and current() would
>>> return it. There may be other layered STTRs but current() would never
>>> return them. That would be easier to implement and IMHO less error prone.
>>>
>>
>> It's not immediately obvious to me this is less error prone...let's say I
>> write library code where I don't know whether or not I'm operating within
>> the context of another STTR. I start up a STTR and use a base::Timer (which
>> we hypothetically have changed to use STTR::current()). Let's say we expect
>> the Timer to exit the STTR. But if the STTR was nested and thus not the
>> "root" (I guess it depends on your definition of root), then this Timer
>> will post a task to the "root" STTR and not the one we're currently running
>> in, and thus won't exit...
>>
>
> I'm not sure I correctly understand the scenario you are describing. And
> we may be talking about different things when we talk about nested thread
> runner. If by nesting you mean invoking MessageLoop::Run() from a task that
> was called form the main task runner (which is not a MessageLoop), then I
> think that's a wrong approach anyway and should be avoided. Again, I don't
> see any good valid scenario when it may be useful to have multiple thread
> runners per one thread.
>

I think your description of a nested task runner is more accurately called
a re-entrant task runner. In your description, it's the same instance. Let
me give an example of a nested STTR:

class PrioritizedSTTR : public STTR {
 public:
  ...

 void Run() {
    while (1) {
      if (!high_priority_sttr_.empty())
        high_priority_sttr_.RunAllPending();
      else if (!medium_priority_sttr_.empty())
        medium_priority_sttr_.RunAllPending();
      else if (!low_priority_sttr_.empty())
        low_priority_sttr_.RunAllPending();
      else
        Sleep(1);
    }
  }

 private:
  BasicSTTR high_priority_sttr_;
  BasicSTTR medium_priority_sttr_;
  BasicSTTR low_priority_sttr_;
};

In this case, during PrioritizedSTTR::Run(), what would you return in
STTR::current()? It's not immediately obvious to me that it's a bad idea to
nest task runners like this (although this is admittedly somewhat of a toy
example).

[Sergey Ulanov, 2012-04-30 19:27:55]

On 2012/04/26 01:00:01, willchan wrote:
> On Wed, Apr 25, 2012 at 5:32 PM, Sergey Ulanov <mailto:sergeyu@chromium.org>
wrote:
> 
> >
> >
> > On Wed, Apr 25, 2012 at 5:11 PM, William Chan (陈智昌)
<mailto:willchan@chromium.org
> > > wrote:
> >
> >> On Wed, Apr 25, 2012 at 4:42 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
> >>
> >>>
> >>> I actually don't think allowing multiple STTR per thread and ensuring
> >>> that STTR::current() always returns the STTR that invoked the current task
> >>> is a good idea. Any examples when this could be useful?
> >>>
> >>
> >> I think we kinda sorta do something like this for testing. In unit_tests,
> >> we often map multiple BrowserThread IDs to the same underlying MessageLoop.
> >>
> >
> > But that's different. In that case there is still have just only one
> > MessageLoop. It's just that the tests lies to the the code being tested,
> > and passes that single MessageLoop pointer as if it was for a different
> > thread.
> >
> 
> >
> >>
> >>
> >>>
> >>> What I suggest (as discribed in my previous reply) is a much simple
> >>> solution: we could have a single "root" STTR per thread and current()
would
> >>> return it. There may be other layered STTRs but current() would never
> >>> return them. That would be easier to implement and IMHO less error prone.
> >>>
> >>
> >> It's not immediately obvious to me this is less error prone...let's say I
> >> write library code where I don't know whether or not I'm operating within
> >> the context of another STTR. I start up a STTR and use a base::Timer (which
> >> we hypothetically have changed to use STTR::current()). Let's say we expect
> >> the Timer to exit the STTR. But if the STTR was nested and thus not the
> >> "root" (I guess it depends on your definition of root), then this Timer
> >> will post a task to the "root" STTR and not the one we're currently running
> >> in, and thus won't exit...
> >>
> >
> > I'm not sure I correctly understand the scenario you are describing. And
> > we may be talking about different things when we talk about nested thread
> > runner. If by nesting you mean invoking MessageLoop::Run() from a task that
> > was called form the main task runner (which is not a MessageLoop), then I
> > think that's a wrong approach anyway and should be avoided. Again, I don't
> > see any good valid scenario when it may be useful to have multiple thread
> > runners per one thread.
> >
> 
> I think your description of a nested task runner is more accurately called
> a re-entrant task runner. In your description, it's the same instance.

No, I wasn't talking about re-entrant task runner, but I see how my description
can be confusing. I was talking about the case when a task called by task runner
A creates a different task runner B and calls B.Run(). Tasks schedule with task
runner A are stalled while task runner B is running.

Another type of nesting that might be actually useful is when there is a main
task runner A and then another task runner B that has different implementation
and works on top of A. B may provide some services in addition to what A
provides. E.g. B may collect statistics about execution time for each task. Your
example with PrioritizedSTTR is somewhat similar, except that instead of one
base task runner A there are three of them. I think that what current() should
return in both of these cases would depend on goals and details of concreate
implemetantions.

Anyway, you suggested earlier to implement SingleThreadExclusiveTaskRunner - I
think it's a good idea. It would make it easier to indicate which task runner
should be returned by current(). I'm not sure about the name though, I suggest
we call it ThreadMainTask, WDYT?

> Let me give an example of a nested STTR:
> 
> class PrioritizedSTTR : public STTR {
>  public:
>   ...
> 
>  void Run() {
>     while (1) {
>       if (!high_priority_sttr_.empty())
>         high_priority_sttr_.RunAllPending();
>       else if (!medium_priority_sttr_.empty())
>         medium_priority_sttr_.RunAllPending();
>       else if (!low_priority_sttr_.empty())
>         low_priority_sttr_.RunAllPending();
>       else
>         Sleep(1);
>     }
>   }
> 
>  private:
>   BasicSTTR high_priority_sttr_;
>   BasicSTTR medium_priority_sttr_;
>   BasicSTTR low_priority_sttr_;
> };
> 
> In this case, during PrioritizedSTTR::Run(), what would you return in
> STTR::current()? It's not immediately obvious to me that it's a bad idea to
> nest task runners like this (although this is admittedly somewhat of a toy
> example).

[Sergey Ulanov, 2012-04-30 20:02:09]

I've uploaded new version that adds ThreadMainTaskRunner interface with
current().

http://codereview.chromium.org/10210008/diff/8001/base/single_thread_task_run...
File base/single_thread_task_runner.cc (right):

http://codereview.chromium.org/10210008/diff/8001/base/single_thread_task_run...
base/single_thread_task_runner.cc:21: return lazy_tls_ptr.Pointer()->Get();
On 2012/04/25 17:27:55, Ami Fischman wrote:
> Can this DCHECK the ret value (see the TODO(darin) in MessageLoop::current()'s
> impl).

Done.

http://codereview.chromium.org/10210008/diff/8001/base/single_thread_task_run...
base/single_thread_task_runner.cc:26: DCHECK(!current() || !new_current);
On 2012/04/25 17:27:55, Ami Fischman wrote:
> WDYT of xor instead of !||! ?
> DCHECK(current() ^ new_current);

^ is bitwise xor, so I'm not sure it would work with pointers properly here. In
any case, I don't see a good reason to prohibit SetCurrent(NULL) when current()
is already set to NULL.

[Ami GONE FROM CHROMIUM, 2012-04-30 23:34:05]

My main original concern is alleviated by the TMTR version (b/c it 's easy to
see that current() can be satisfied by it and not easily by a superclass).  I am
still left uneasy by the API but as long as DCHECKs enforce that it is used only
when appropriate, not speculatively (say by not DCHECKing in current()), that's
ignorable.

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
File base/thread_main_task_runner.cc (right):

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
base/thread_main_task_runner.cc:28: DCHECK(!current() || !new_current);
I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is called,
since current() DCHECK's its own result.

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
base/thread_main_task_runner.cc:28: DCHECK(!current() || !new_current);
(assuming you s/current()/lazy_tls_ptr.Pointer()->Get()/ for the previous
comment)
Can you think of a case where it's not a programmer error to call
SetCurrent(NULL) without a previous not-NULL value being set?

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
File base/thread_main_task_runner.h (right):

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
base/thread_main_task_runner.h:18: // insterface that corresponds to the current
thread.
typo: "insterface"

[Sergey Ulanov, 2012-05-01 00:34:54]

also added unittest to verify that MessageLoop sets
ThreadMainTaskRunner::current().

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
File base/thread_main_task_runner.cc (right):

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
base/thread_main_task_runner.cc:28: DCHECK(!current() || !new_current);
On 2012/04/30 23:34:05, Ami Fischman wrote:
> I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is called,
> since current() DCHECK's its own result.

Oops, I didn't test this code after adding DCHECK in current(). Fixed now.

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
base/thread_main_task_runner.cc:28: DCHECK(!current() || !new_current);
On 2012/04/30 23:34:05, Ami Fischman wrote:
> (assuming you s/current()/lazy_tls_ptr.Pointer()->Get()/ for the previous
> comment)
> Can you think of a case where it's not a programmer error to call
> SetCurrent(NULL) without a previous not-NULL value being set?
No. Ok, I changed this code to check this case too, but without XOR.

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
File base/thread_main_task_runner.h (right):

http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
base/thread_main_task_runner.h:18: // insterface that corresponds to the current
thread.
On 2012/04/30 23:34:05, Ami Fischman wrote:
> typo: "insterface"

Done.

[willchan no longer on Chromium, 2012-05-01 00:38:19]

OK, I'm sold on TMTR. Can you explain why you don't set the thread local in the
constructor/destructor of TMTR?

On 2012/05/01 00:34:54, sergeyu wrote:
> also added unittest to verify that MessageLoop sets
> ThreadMainTaskRunner::current().
> 
>
http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
> File base/thread_main_task_runner.cc (right):
> 
>
http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
> base/thread_main_task_runner.cc:28: DCHECK(!current() || !new_current);
> On 2012/04/30 23:34:05, Ami Fischman wrote:
> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is
called,
> > since current() DCHECK's its own result.
> 
> Oops, I didn't test this code after adding DCHECK in current(). Fixed now.
> 
>
http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
> base/thread_main_task_runner.cc:28: DCHECK(!current() || !new_current);
> On 2012/04/30 23:34:05, Ami Fischman wrote:
> > (assuming you s/current()/lazy_tls_ptr.Pointer()->Get()/ for the previous
> > comment)
> > Can you think of a case where it's not a programmer error to call
> > SetCurrent(NULL) without a previous not-NULL value being set?
> No. Ok, I changed this code to check this case too, but without XOR.
> 
>
http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
> File base/thread_main_task_runner.h (right):
> 
>
http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runn...
> base/thread_main_task_runner.h:18: // insterface that corresponds to the
current
> thread.
> On 2012/04/30 23:34:05, Ami Fischman wrote:
> > typo: "insterface"
> 
> Done.

[Sergey Ulanov, 2012-05-01 00:46:45]

I cannot do it in the destructor because the object is ref-counted and thus
we don't any guarantees about the thread on which it's going to be
destroyed. Because we cannot reset current() in the destructor, it's better
not to set it the constructor.

On Mon, Apr 30, 2012 at 5:38 PM, <willchan@chromium.org> wrote:

> OK, I'm sold on TMTR. Can you explain why you don't set the thread local
> in the
> constructor/destructor of TMTR?
>
>
> On 2012/05/01 00:34:54, sergeyu wrote:
>
>> also added unittest to verify that MessageLoop sets
>> ThreadMainTaskRunner::current(**).
>>
>
>
> http://codereview.chromium.**org/10210008/diff/21002/base/**
>
thread_main_task_runner.cc<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc>
>
>> File base/thread_main_task_runner.**cc (right):
>>
>
>
> http://codereview.chromium.**org/10210008/diff/21002/base/**
>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>
>> base/thread_main_task_runner.**cc:28: DCHECK(!current() || !new_current);
>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is
>>
> called,
>
>> > since current() DCHECK's its own result.
>>
>
>  Oops, I didn't test this code after adding DCHECK in current(). Fixed now.
>>
>
>
> http://codereview.chromium.**org/10210008/diff/21002/base/**
>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>
>> base/thread_main_task_runner.**cc:28: DCHECK(!current() || !new_current);
>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>> > (assuming you s/current()/lazy_tls_ptr.**Pointer()->Get()/ for the
>> previous
>> > comment)
>> > Can you think of a case where it's not a programmer error to call
>> > SetCurrent(NULL) without a previous not-NULL value being set?
>> No. Ok, I changed this code to check this case too, but without XOR.
>>
>
>
> http://codereview.chromium.**org/10210008/diff/21002/base/**
>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h>
>
>> File base/thread_main_task_runner.h (right):
>>
>
>
> http://codereview.chromium.**org/10210008/diff/21002/base/**
>
thread_main_task_runner.h#**newcode18<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h#newcode18>
>
>> base/thread_main_task_runner.**h:18: // insterface that corresponds to
>> the
>>
> current
>
>> thread.
>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>> > typo: "insterface"
>>
>
>  Done.
>>
>
>
>
>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>

[willchan no longer on Chromium, 2012-05-01 01:13:51]

The comment about the destructor makes sense. I'm not sure I think we
shouldn't set it in the constructor. Why don't we set it in the constructor
and provide a protected API to set it back to NULL? Basically, I'm looking
for ways for us to restrict abuse, by making it impossible to create
multiple TMTR instances on the same thread.

On Mon, Apr 30, 2012 at 5:46 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> I cannot do it in the destructor because the object is ref-counted and
> thus we don't any guarantees about the thread on which it's going to be
> destroyed. Because we cannot reset current() in the destructor, it's better
> not to set it the constructor.
>
>
> On Mon, Apr 30, 2012 at 5:38 PM, <willchan@chromium.org> wrote:
>
>> OK, I'm sold on TMTR. Can you explain why you don't set the thread local
>> in the
>> constructor/destructor of TMTR?
>>
>>
>> On 2012/05/01 00:34:54, sergeyu wrote:
>>
>>> also added unittest to verify that MessageLoop sets
>>> ThreadMainTaskRunner::current(**).
>>>
>>
>>
>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>
thread_main_task_runner.cc<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc>
>>
>>> File base/thread_main_task_runner.**cc (right):
>>>
>>
>>
>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>
>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>> !new_current);
>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is
>>>
>> called,
>>
>>> > since current() DCHECK's its own result.
>>>
>>
>>  Oops, I didn't test this code after adding DCHECK in current(). Fixed
>>> now.
>>>
>>
>>
>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>
>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>> !new_current);
>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>> > (assuming you s/current()/lazy_tls_ptr.**Pointer()->Get()/ for the
>>> previous
>>> > comment)
>>> > Can you think of a case where it's not a programmer error to call
>>> > SetCurrent(NULL) without a previous not-NULL value being set?
>>> No. Ok, I changed this code to check this case too, but without XOR.
>>>
>>
>>
>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h>
>>
>>> File base/thread_main_task_runner.h (right):
>>>
>>
>>
>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>
thread_main_task_runner.h#**newcode18<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h#newcode18>
>>
>>> base/thread_main_task_runner.**h:18: // insterface that corresponds to
>>> the
>>>
>> current
>>
>>> thread.
>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>> > typo: "insterface"
>>>
>>
>>  Done.
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>>
>
>

[Ami GONE FROM CHROMIUM, 2012-05-01 01:32:37]

On Mon, Apr 30, 2012 at 6:13 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> provide a protected API to set it back to NULL?


Crazy thought: what does that buy us?  Why not just leave the TLS to be
collected by thread cleanup?

[willchan no longer on Chromium, 2012-05-01 01:40:24]

Interesting thought. Maybe you're exactly right! The only thing I can thing
of is sometimes we use the system worker pool, like Windows, so it's
possible to reuse a thread. Do we really want to prevent thread reuse?

On Mon, Apr 30, 2012 at 6:32 PM, Ami Fischman <fischman@chromium.org> wrote:

> On Mon, Apr 30, 2012 at 6:13 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> provide a protected API to set it back to NULL?
>
>
> Crazy thought: what does that buy us?  Why not just leave the TLS to be
> collected by thread cleanup?
>
>

[Sergey Ulanov, 2012-05-01 01:41:32]

On Mon, Apr 30, 2012 at 6:13 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> The comment about the destructor makes sense. I'm not sure I think we
> shouldn't set it in the constructor. Why don't we set it in the constructor
> and provide a protected API to set it back to NULL? Basically, I'm looking
> for ways for us to restrict abuse, by making it impossible to create
> multiple TMTR instances on the same thread.
>

Problem is that there must always be a separate object (like MessageLoop)
that holds a reference to ThreadMainTaskRunner. If the constructor was
settings current() then it would be very easy to forget about this and then
either the task runner will be leaked or destroyed prematurely.


>
>
> On Mon, Apr 30, 2012 at 5:46 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>> I cannot do it in the destructor because the object is ref-counted and
>> thus we don't any guarantees about the thread on which it's going to be
>> destroyed. Because we cannot reset current() in the destructor, it's better
>> not to set it the constructor.
>>
>>
>> On Mon, Apr 30, 2012 at 5:38 PM, <willchan@chromium.org> wrote:
>>
>>> OK, I'm sold on TMTR. Can you explain why you don't set the thread local
>>> in the
>>> constructor/destructor of TMTR?
>>>
>>>
>>> On 2012/05/01 00:34:54, sergeyu wrote:
>>>
>>>> also added unittest to verify that MessageLoop sets
>>>> ThreadMainTaskRunner::current(**).
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>
thread_main_task_runner.cc<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc>
>>>
>>>> File base/thread_main_task_runner.**cc (right):
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>>
>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>>> !new_current);
>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is
>>>>
>>> called,
>>>
>>>> > since current() DCHECK's its own result.
>>>>
>>>
>>>  Oops, I didn't test this code after adding DCHECK in current(). Fixed
>>>> now.
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>>
>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>>> !new_current);
>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>> > (assuming you s/current()/lazy_tls_ptr.**Pointer()->Get()/ for the
>>>> previous
>>>> > comment)
>>>> > Can you think of a case where it's not a programmer error to call
>>>> > SetCurrent(NULL) without a previous not-NULL value being set?
>>>> No. Ok, I changed this code to check this case too, but without XOR.
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h>
>>>
>>>> File base/thread_main_task_runner.h (right):
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>
thread_main_task_runner.h#**newcode18<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h#newcode18>
>>>
>>>> base/thread_main_task_runner.**h:18: // insterface that corresponds to
>>>> the
>>>>
>>> current
>>>
>>>> thread.
>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>> > typo: "insterface"
>>>>
>>>
>>>  Done.
>>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>>>
>>
>>
>

[Sergey Ulanov, 2012-05-01 01:43:13]

On Mon, Apr 30, 2012 at 6:41 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> On Mon, Apr 30, 2012 at 6:13 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> The comment about the destructor makes sense. I'm not sure I think we
>> shouldn't set it in the constructor. Why don't we set it in the constructor
>> and provide a protected API to set it back to NULL? Basically, I'm looking
>> for ways for us to restrict abuse, by making it impossible to create
>> multiple TMTR instances on the same thread.
>>
>
> Problem is that there must always be a separate object (like MessageLoop)
> that holds a reference to ThreadMainTaskRunner. If the constructor was
> settings current() then it would be very easy to forget about this and then
> either the task runner will be leaked or destroyed prematurely.
>

I've added DCHECK in the constructor that verifies that current() == NULL -
that will ensure that one cannot create more than one ThreadMainTaskRunner
per thread. What do you think about that.


>
>
>>
>>
>> On Mon, Apr 30, 2012 at 5:46 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>>
>>> I cannot do it in the destructor because the object is ref-counted and
>>> thus we don't any guarantees about the thread on which it's going to be
>>> destroyed. Because we cannot reset current() in the destructor, it's better
>>> not to set it the constructor.
>>>
>>>
>>> On Mon, Apr 30, 2012 at 5:38 PM, <willchan@chromium.org> wrote:
>>>
>>>> OK, I'm sold on TMTR. Can you explain why you don't set the thread
>>>> local in the
>>>> constructor/destructor of TMTR?
>>>>
>>>>
>>>> On 2012/05/01 00:34:54, sergeyu wrote:
>>>>
>>>>> also added unittest to verify that MessageLoop sets
>>>>> ThreadMainTaskRunner::current(**).
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>
thread_main_task_runner.cc<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc>
>>>>
>>>>> File base/thread_main_task_runner.**cc (right):
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>>>
>>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>>>> !new_current);
>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>>> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent() is
>>>>>
>>>> called,
>>>>
>>>>> > since current() DCHECK's its own result.
>>>>>
>>>>
>>>>  Oops, I didn't test this code after adding DCHECK in current(). Fixed
>>>>> now.
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>>>
>>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>>>> !new_current);
>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>>> > (assuming you s/current()/lazy_tls_ptr.**Pointer()->Get()/ for the
>>>>> previous
>>>>> > comment)
>>>>> > Can you think of a case where it's not a programmer error to call
>>>>> > SetCurrent(NULL) without a previous not-NULL value being set?
>>>>> No. Ok, I changed this code to check this case too, but without XOR.
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h>
>>>>
>>>>> File base/thread_main_task_runner.h (right):
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>
thread_main_task_runner.h#**newcode18<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h#newcode18>
>>>>
>>>>> base/thread_main_task_runner.**h:18: // insterface that corresponds
>>>>> to the
>>>>>
>>>> current
>>>>
>>>>> thread.
>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>>> > typo: "insterface"
>>>>>
>>>>
>>>>  Done.
>>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>>>>
>>>
>>>
>>
>

[Sergey Ulanov, 2012-05-01 01:44:46]

On Mon, Apr 30, 2012 at 6:32 PM, Ami Fischman <fischman@chromium.org> wrote:

> On Mon, Apr 30, 2012 at 6:13 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> provide a protected API to set it back to NULL?
>
>
> Crazy thought: what does that buy us?  Why not just leave the TLS to be
> collected by thread cleanup?
>

That would make this code harder to test. Also see my other mail - the
object is ref-counted and we cannot store counted references in
thread-local storage, this means that there still must be a separate object
that holds a reference while the object is alive.

[willchan no longer on Chromium, 2012-05-01 02:05:07]

On Mon, Apr 30, 2012 at 6:42 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> On Mon, Apr 30, 2012 at 6:41 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>> On Mon, Apr 30, 2012 at 6:13 PM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> The comment about the destructor makes sense. I'm not sure I think we
>>> shouldn't set it in the constructor. Why don't we set it in the constructor
>>> and provide a protected API to set it back to NULL? Basically, I'm looking
>>> for ways for us to restrict abuse, by making it impossible to create
>>> multiple TMTR instances on the same thread.
>>>
>>
>> Problem is that there must always be a separate object (like MessageLoop)
>> that holds a reference to ThreadMainTaskRunner. If the constructor was
>> settings current() then it would be very easy to forget about this and then
>> either the task runner will be leaked or destroyed prematurely.
>>
>
What's wrong with having a separate object that holds the reference? I
think that's what we do with MessageLoop and MessageLoopProxy right now.
MessageLoop constructs the MessageLoopProxy. Or do you think we should
change how MessageLoop and MessageLoopProxy interface too?


>
> I've added DCHECK in the constructor that verifies that current() == NULL
> - that will ensure that one cannot create more than one
> ThreadMainTaskRunner per thread. What do you think about that.
>

I think that if someone were lame they would set current to NULL and then
create their TMTR. I prefer not leaving holes in base/ APIs, people seem to
abuse them. The more we can restrict that, the better.


>
>
>>
>>
>>>
>>>
>>> On Mon, Apr 30, 2012 at 5:46 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>>>
>>>> I cannot do it in the destructor because the object is ref-counted and
>>>> thus we don't any guarantees about the thread on which it's going to be
>>>> destroyed. Because we cannot reset current() in the destructor, it's
better
>>>> not to set it the constructor.
>>>>
>>>>
>>>> On Mon, Apr 30, 2012 at 5:38 PM, <willchan@chromium.org> wrote:
>>>>
>>>>> OK, I'm sold on TMTR. Can you explain why you don't set the thread
>>>>> local in the
>>>>> constructor/destructor of TMTR?
>>>>>
>>>>>
>>>>> On 2012/05/01 00:34:54, sergeyu wrote:
>>>>>
>>>>>> also added unittest to verify that MessageLoop sets
>>>>>> ThreadMainTaskRunner::current(**).
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>>
thread_main_task_runner.cc<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc>
>>>>>
>>>>>> File base/thread_main_task_runner.**cc (right):
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>>>>
>>>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>>>>> !new_current);
>>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>>>> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent()
>>>>>> is
>>>>>>
>>>>> called,
>>>>>
>>>>>> > since current() DCHECK's its own result.
>>>>>>
>>>>>
>>>>>  Oops, I didn't test this code after adding DCHECK in current(). Fixed
>>>>>> now.
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
>>>>>
>>>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
>>>>>> !new_current);
>>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>>>> > (assuming you s/current()/lazy_tls_ptr.**Pointer()->Get()/ for the
>>>>>> previous
>>>>>> > comment)
>>>>>> > Can you think of a case where it's not a programmer error to call
>>>>>> > SetCurrent(NULL) without a previous not-NULL value being set?
>>>>>> No. Ok, I changed this code to check this case too, but without XOR.
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h>
>>>>>
>>>>>> File base/thread_main_task_runner.h (right):
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
>>>>>
thread_main_task_runner.h#**newcode18<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h#newcode18>
>>>>>
>>>>>> base/thread_main_task_runner.**h:18: // insterface that corresponds
>>>>>> to the
>>>>>>
>>>>> current
>>>>>
>>>>>> thread.
>>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
>>>>>> > typo: "insterface"
>>>>>>
>>>>>
>>>>>  Done.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>>>>>
>>>>
>>>>
>>>
>>
>

[Sergey Ulanov, 2012-05-01 19:23:23]

On 2012/05/01 02:05:07, willchan wrote:
> On Mon, Apr 30, 2012 at 6:42 PM, Sergey Ulanov <mailto:sergeyu@chromium.org>
wrote:
> 
> > On Mon, Apr 30, 2012 at 6:41 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
> >> Problem is that there must always be a separate object (like MessageLoop)
> >> that holds a reference to ThreadMainTaskRunner. If the constructor was
> >> settings current() then it would be very easy to forget about this and then
> >> either the task runner will be leaked or destroyed prematurely.
> >>
> >
> What's wrong with having a separate object that holds the reference? I
> think that's what we do with MessageLoop and MessageLoopProxy right now.
> MessageLoop constructs the MessageLoopProxy. Or do you think we should
> change how MessageLoop and MessageLoopProxy interface too?

There is nothing wrong with having a separate object that holds the reference to
this interface. It's just if the constructor sets current() then it becomes easy
to overlook that such object is needed and so easy to leak TMTL.

> >
> > I've added DCHECK in the constructor that verifies that current() == NULL
> > - that will ensure that one cannot create more than one
> > ThreadMainTaskRunner per thread. What do you think about that.
> >
> 
> I think that if someone were lame they would set current to NULL and then
> create their TMTR. I prefer not leaving holes in base/ APIs, people seem to
> abuse them. The more we can restrict that, the better.

OK, I've moved current() initialization to constructor and added protected
Detach() method that implementations must call before thread is destroyed.

> 
> 
> >
> >
> >>
> >>
> >>>
> >>>
> >>> On Mon, Apr 30, 2012 at 5:46 PM, Sergey Ulanov
<sergeyu@chromium.org>wrote:
> >>>
> >>>> I cannot do it in the destructor because the object is ref-counted and
> >>>> thus we don't any guarantees about the thread on which it's going to be
> >>>> destroyed. Because we cannot reset current() in the destructor, it's
> better
> >>>> not to set it the constructor.
> >>>>
> >>>>
> >>>> On Mon, Apr 30, 2012 at 5:38 PM, <mailto:willchan@chromium.org> wrote:
> >>>>
> >>>>> OK, I'm sold on TMTR. Can you explain why you don't set the thread
> >>>>> local in the
> >>>>> constructor/destructor of TMTR?
> >>>>>
> >>>>>
> >>>>> On 2012/05/01 00:34:54, sergeyu wrote:
> >>>>>
> >>>>>> also added unittest to verify that MessageLoop sets
> >>>>>> ThreadMainTaskRunner::current(**).
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
> >>>>>
>
thread_main_task_runner.cc<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc>
> >>>>>
> >>>>>> File base/thread_main_task_runner.**cc (right):
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
> >>>>>
>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
> >>>>>
> >>>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
> >>>>>> !new_current);
> >>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
> >>>>>> > I'm surprised this doesn't DCHECK-fail the first time SetCurrent()
> >>>>>> is
> >>>>>>
> >>>>> called,
> >>>>>
> >>>>>> > since current() DCHECK's its own result.
> >>>>>>
> >>>>>
> >>>>>  Oops, I didn't test this code after adding DCHECK in current(). Fixed
> >>>>>> now.
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
> >>>>>
>
thread_main_task_runner.cc#**newcode28<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.cc#newcode28>
> >>>>>
> >>>>>> base/thread_main_task_runner.**cc:28: DCHECK(!current() ||
> >>>>>> !new_current);
> >>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
> >>>>>> > (assuming you s/current()/lazy_tls_ptr.**Pointer()->Get()/ for the
> >>>>>> previous
> >>>>>> > comment)
> >>>>>> > Can you think of a case where it's not a programmer error to call
> >>>>>> > SetCurrent(NULL) without a previous not-NULL value being set?
> >>>>>> No. Ok, I changed this code to check this case too, but without XOR.
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
> >>>>>
>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h>
> >>>>>
> >>>>>> File base/thread_main_task_runner.h (right):
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/10210008/diff/21002/base/**
> >>>>>
>
thread_main_task_runner.h#**newcode18<http://codereview.chromium.org/10210008/diff/21002/base/thread_main_task_runner.h#newcode18>
> >>>>>
> >>>>>> base/thread_main_task_runner.**h:18: // insterface that corresponds
> >>>>>> to the
> >>>>>>
> >>>>> current
> >>>>>
> >>>>>> thread.
> >>>>>> On 2012/04/30 23:34:05, Ami Fischman wrote:
> >>>>>> > typo: "insterface"
> >>>>>>
> >>>>>
> >>>>>  Done.
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
>
http://codereview.chromium.**org/10210008/%3Chttp://codereview.chromium.org/1...>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >

[willchan no longer on Chromium, 2012-05-02 00:17:12]

LGTM, please remember to use the task_runner_test_template.h in your
implementation's test.

http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runn...
File base/thread_main_task_runner.h (right):

http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runn...
base/thread_main_task_runner.h:14: // ThreadMainTaskRunner is a
SignleThreadTaskRunner that is used for
SingleThreadTaskRunner

[Sergey Ulanov, 2012-05-02 00:30:35]

There is now problem that BrowserThreadImpl creates MessageLoopProxy instances
that are different from MessageLoopImpl. This DCHECK's many tests. It needs to
be fixed before I can land this change. I'll follow up with jam@

http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runn...
File base/thread_main_task_runner.h (right):

http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runn...
base/thread_main_task_runner.h:14: // ThreadMainTaskRunner is a
SignleThreadTaskRunner that is used for
On 2012/05/02 00:17:12, willchan wrote:
> SingleThreadTaskRunner

Done.

[willchan no longer on Chromium, 2012-05-02 00:32:59]

I have a pending CL to fix that. But I didn't account for the remoting/
cases. I can upload a change for you to finish if you want.

On Tue, May 1, 2012 at 5:30 PM, <sergeyu@chromium.org> wrote:

> There is now problem that BrowserThreadImpl creates MessageLoopProxy
> instances
> that are different from MessageLoopImpl. This DCHECK's many tests. It
> needs to
> be fixed before I can land this change. I'll follow up with jam@
>
>
>
> http://codereview.chromium.**org/10210008/diff/30011/base/**
>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runner.h>
> File base/thread_main_task_runner.h (right):
>
> http://codereview.chromium.**org/10210008/diff/30011/base/**
>
thread_main_task_runner.h#**newcode14<http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runner.h#newcode14>
> base/thread_main_task_runner.**h:14: // ThreadMainTaskRunner is a
> SignleThreadTaskRunner that is used for
> On 2012/05/02 00:17:12, willchan wrote:
>
>> SingleThreadTaskRunner
>>
>
> Done.
>
>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>

[willchan no longer on Chromium, 2012-05-02 00:36:16]

Apparently I was wrong. I only did the merging (and didn't even delete the
old message_loop_proxy_impl files). If you want it, it's here:
http://codereview.chromium.org/10278009/.

On Tue, May 1, 2012 at 5:32 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> I have a pending CL to fix that. But I didn't account for the remoting/
> cases. I can upload a change for you to finish if you want.
>
>
> On Tue, May 1, 2012 at 5:30 PM, <sergeyu@chromium.org> wrote:
>
>> There is now problem that BrowserThreadImpl creates MessageLoopProxy
>> instances
>> that are different from MessageLoopImpl. This DCHECK's many tests. It
>> needs to
>> be fixed before I can land this change. I'll follow up with jam@
>>
>>
>>
>> http://codereview.chromium.**org/10210008/diff/30011/base/**
>>
thread_main_task_runner.h<http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runner.h>
>> File base/thread_main_task_runner.h (right):
>>
>> http://codereview.chromium.**org/10210008/diff/30011/base/**
>>
thread_main_task_runner.h#**newcode14<http://codereview.chromium.org/10210008/diff/30011/base/thread_main_task_runner.h#newcode14>
>> base/thread_main_task_runner.**h:14: // ThreadMainTaskRunner is a
>> SignleThreadTaskRunner that is used for
>> On 2012/05/02 00:17:12, willchan wrote:
>>
>>> SingleThreadTaskRunner
>>>
>>
>> Done.
>>
>>
http://codereview.chromium.**org/10210008/<http://codereview.chromium.org/102...
>>
>
>

[Sergey Ulanov, 2012-05-02 00:42:19]

On Tue, May 1, 2012 at 5:36 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Apparently I was wrong. I only did the merging (and didn't even delete the
> old message_loop_proxy_impl files). If you want it, it's here:
> http://codereview.chromium.org/10278009/.


I was planning to land this CL first and then switch remoting code to using
the new ThreadMainTaskRunner. I think we need to do it in the following
sequence:
 1. Fix BrowserThread so that it doesn't create MessageLoopProxy.
 2. Land this CL.
 3. Fix Remoting code so that it doesn't create MessageLoopProxy and
uses ThreadMainTaskRunner instead.
 4. Land your CL that merges MessageLoopProxyImpl into MessageLoopProxy.

How does that sound?

[willchan no longer on Chromium, 2012-05-02 00:43:25]

I see. Yes, that sounds correct to me.

On Tue, May 1, 2012 at 5:41 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> On Tue, May 1, 2012 at 5:36 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> Apparently I was wrong. I only did the merging (and didn't even delete
>> the old message_loop_proxy_impl files). If you want it, it's here:
>> http://codereview.chromium.org/10278009/.
>
>
> I was planning to land this CL first and then switch remoting code to
> using the new ThreadMainTaskRunner. I think we need to do it in the
> following sequence:
>  1. Fix BrowserThread so that it doesn't create MessageLoopProxy.
>  2. Land this CL.
>  3. Fix Remoting code so that it doesn't create MessageLoopProxy and
> uses ThreadMainTaskRunner instead.
>  4. Land your CL that merges MessageLoopProxyImpl into MessageLoopProxy.
>
> How does that sound?
>

[Sergey Ulanov, 2012-05-02 00:59:06]

Ok, I've put together a CL to fix (1): http://codereview.chromium.org/10291011

On 2012/05/02 00:43:25, willchan wrote:
> I see. Yes, that sounds correct to me.
> 
> On Tue, May 1, 2012 at 5:41 PM, Sergey Ulanov <mailto:sergeyu@chromium.org>
wrote:
> 
> > On Tue, May 1, 2012 at 5:36 PM, William Chan (陈智昌)
> <willchan@chromium.org>wrote:
> >
> >> Apparently I was wrong. I only did the merging (and didn't even delete
> >> the old message_loop_proxy_impl files). If you want it, it's here:
> >> http://codereview.chromium.org/10278009/.
> >
> >
> > I was planning to land this CL first and then switch remoting code to
> > using the new ThreadMainTaskRunner. I think we need to do it in the
> > following sequence:
> >  1. Fix BrowserThread so that it doesn't create MessageLoopProxy.
> >  2. Land this CL.
> >  3. Fix Remoting code so that it doesn't create MessageLoopProxy and
> > uses ThreadMainTaskRunner instead.
> >  4. Land your CL that merges MessageLoopProxyImpl into MessageLoopProxy.
> >
> > How does that sound?
> >

[akalin, 2012-05-09 21:40:05]

Is this CL obsoleted by http://codereview.chromium.org/10380016/ ?

[Sergey Ulanov, 2012-05-09 22:06:48]

On 2012/05/09 21:40:05, akalin wrote:
> Is this CL obsoleted by http://codereview.chromium.org/10380016/ ?

Yes. Closing it.