Add WorkerPool::PostTaskAndReply and use in DHCP code.

Add WorkerPool::PostTaskAndReply and use in DHCP code.

This factors out the PostTaskAndReply implementation out of MessageLoopProxy so that it can be used for any destination thread accessible via a PostTask-like interface, and uses that code from both MessageLoopProxy and WorkerPool.

The DhcpProxyScriptFetcherWin and DhcpProxyScriptAdapterFetcher classes were both using a PostTaskAndReply-like mechanism with a WorkerPool thread, and on inspection it looks like there are several places in net/ where this is done, and this motivated the larger change (vs. patch set 1 which was a mechanical switch to base::Bind from NewRunnableMethod).

BUG=97516
TEST=net_unittests

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

[Jói, 2011-10-05 16:35:27]

ajwong: Main reviewer for base/ changes since they are all about
PostTaskAndReply.

willchan: Main reviewer for net/, and OWNERS approval for both base/ and net/.

Cheers,
Jói

[awong, 2011-10-05 17:27:01]

Thanks for doing this!

Haven't looked at the net stuff, but need to run and wanted to send initial
comments.

Could you also add crbug.com/97516 to the BUG=?  That's the bug for making
WorkerPool understand PostTaskAndReply.

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
File base/threading/post_task_and_reply_impl.cc (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.cc:7: #include
"base/message_loop_proxy.h"
Add #include bind.h and location.h for IWYU.

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
File base/threading/post_task_and_reply_impl.h (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.h:16: 
Put this in the base::internal namespace?

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.h:26: class PostTaskAndReplyImpl {
I think it'd be simpler to just loft the concrete PostTaskAndReply() class into
this file.

Then in MessageLoopProxy(), and WorkerPool(), you can just new/post it as you
want. No need to indirect through an interface.

http://codereview.chromium.org/8139028/diff/3001/base/threading/worker_pool_u...
File base/threading/worker_pool_unittest.cc (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/worker_pool_u...
base/threading/worker_pool_unittest.cc:58:
MessageLoop::current()->RunAllPending();
Feels cleaner to me to have the waiting logic out in the main test, pass the
messageloop into the test directly, and then call ml->Quit() (as opposed to
ML::current()->Quit()) inside the test instead.

Otherwise reading the top-level TEST_F, I ask "why did we just declare a
MessageLoop and not use it anywhere?  Or right...people can touch the global..".

http://codereview.chromium.org/8139028/diff/3001/base/threading/worker_pool_w...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/worker_pool_w...
base/threading/worker_pool_win.cc:111: from_here, task, reply);
Yeah, if PostTaskAndReply were in a header file, this could just be

PostTAskAndReply relay = new PostTaskAndReply(from_here, task, reply);

return PostTask(from_here, base::Bind(&PostTaskAndReply::Run,
base::Unretained(relay), task_is_slow));

[Jói, 2011-10-06 14:20:31]

Thanks, I've uploaded a new version that addresses some of your review comments
(others are addressed below).  This version also fixes flakiness in one of the
unit tests.

Cheers,
Jói

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
File base/threading/post_task_and_reply_impl.cc (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.cc:7: #include
"base/message_loop_proxy.h"
On 2011/10/05 17:27:01, awong wrote:
> Add #include bind.h and location.h for IWYU.

Done.

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
File base/threading/post_task_and_reply_impl.h (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.h:16: 
On 2011/10/05 17:27:01, awong wrote:
> Put this in the base::internal namespace?

Yes, I like that.

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.h:26: class PostTaskAndReplyImpl {
On 2011/10/05 17:27:01, awong wrote:
> I think it'd be simpler to just loft the concrete PostTaskAndReply() class
into
> this file.
> 
> Then in MessageLoopProxy(), and WorkerPool(), you can just new/post it as you
> want. No need to indirect through an interface.

That way there's duplication of code and the possibility of having to update two
places in the future to fix a bug or introduce new functionality for
PostTaskAndReply.  My preference would be to leave as is.

http://codereview.chromium.org/8139028/diff/3001/base/threading/worker_pool_u...
File base/threading/worker_pool_unittest.cc (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/worker_pool_u...
base/threading/worker_pool_unittest.cc:58:
MessageLoop::current()->RunAllPending();
On 2011/10/05 17:27:01, awong wrote:
> Feels cleaner to me to have the waiting logic out in the main test, pass the
> messageloop into the test directly, and then call ml->Quit() (as opposed to
> ML::current()->Quit()) inside the test instead.
> 
> Otherwise reading the top-level TEST_F, I ask "why did we just declare a
> MessageLoop and not use it anywhere?  Or right...people can touch the
global..".

Done.

[awong, 2011-10-06 19:53:52]

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
File base/threading/post_task_and_reply_impl.h (right):

http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
base/threading/post_task_and_reply_impl.h:26: class PostTaskAndReplyImpl {
On 2011/10/06 14:20:31, Jói wrote:
> On 2011/10/05 17:27:01, awong wrote:
> > I think it'd be simpler to just loft the concrete PostTaskAndReply() class
> into
> > this file.
> > 
> > Then in MessageLoopProxy(), and WorkerPool(), you can just new/post it as
you
> > want. No need to indirect through an interface.
> 
> That way there's duplication of code and the possibility of having to update
two
> places in the future to fix a bug or introduce new functionality for
> PostTaskAndReply.  My preference would be to leave as is.

I think I'm still in favor of removing this template method pattern.  The shared
code is pretty short and is just the standard pattern of "delete if post fails."
 I don't see any reason it would diverge in the future, so the removal of an
inheritance relation + a virtual dispatch seems worth it.

[Jói, 2011-10-07 17:17:08]

> The shared code is pretty short and is just the standard pattern of [...]

This is often true and then it changes down the line, and when it does
neither you nor I might be involved and recall that there are two
copies of the code :)  E.g. initialization of the Relay object might
change, then later there might be some subtle bug with that, etc. etc.
 I'm not saying it necessarily _will_ happen, it's just that I object
to code duplication in principle and this way a fix or improvement in
one place but not the other _can't_ happen.

> [...] the removal of an inheritance relation + a virtual dispatch
> seems worth it.

The inheritance relation is probably free from a performance
standpoint.  The virtual dispatch isn't, but I doubt that it's more
expensive in the grand scheme of things than having two copies of the
function (and thus making our executable larger), or at least I'd say
it's close enough to call.

Cheers,
Jói


On Thu, Oct 6, 2011 at 7:53 PM,  <ajwong@chromium.org> wrote:
>
>
http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
> File base/threading/post_task_and_reply_impl.h (right):
>
>
http://codereview.chromium.org/8139028/diff/3001/base/threading/post_task_and...
> base/threading/post_task_and_reply_impl.h:26: class PostTaskAndReplyImpl
> {
> On 2011/10/06 14:20:31, Jói wrote:
>>
>> On 2011/10/05 17:27:01, awong wrote:
>> > I think it'd be simpler to just loft the concrete PostTaskAndReply()
>
> class
>>
>> into
>> > this file.
>> >
>> > Then in MessageLoopProxy(), and WorkerPool(), you can just new/post
>
> it as you
>>
>> > want. No need to indirect through an interface.
>
>> That way there's duplication of code and the possibility of having to
>
> update two
>>
>> places in the future to fix a bug or introduce new functionality for
>> PostTaskAndReply.  My preference would be to leave as is.
>
> I think I'm still in favor of removing this template method pattern.
> The shared code is pretty short and is just the standard pattern of
> "delete if post fails."  I don't see any reason it would diverge in the
> future, so the removal of an inheritance relation + a virtual dispatch
> seems worth it.
>
> http://codereview.chromium.org/8139028/
>

[awong, 2011-10-07 17:53:22]

Fair enough about the possible future divergence.  I agree it is a tossup.

There really needs to be some sort of interface that allows one to accept any
sort of template executor.

I think Will and Darin have talked about this.  Maybe it's time to make that
happen...

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:63: scoped_refptr<DhcpQuery>
dhcp_query(ImplCreateDhcpQuery());
I wonder if it would be possible to not bother with the refcount here if you
delete dhcp_query on the reply.

Might make your unittest's lifetime management slightly harder though.

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:68: dhcp_query.get(),
FYI, You no longer need the .get().  Submitted CL yesterday to fix.

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:72:
base::WorkerPool::PostTaskAndReply(
Not sure this would be cleaner, but figured I'd point it out.

You could kill off the AdapterQuery class completely and do

set<string>* adapter_names = new set<string>();
PostTaskAndReply(
    FROM_HERE,
    base::Bind(
        &ImplGetCandidateAdapterNames
        adapter_names,
        last_query_.get()),
    base::Bind(
        &DhcpProxyScriptFetcherWin::OnGetCandidateAdapterNamesDone,
        AsWeakPtr(),
        adapter_names,
        last_query_.get());

And then delete adapter_names in the reply.

Next week, I expect to add another wrapper that makes it so you could pass 
base::Owned(adapter_names) to the OnGetCandidateAdapterNamesDone bind, and not
even bother with the delete.

Again, this would mess with your unittest error injection.

Just a thought. Feel free to ignore.

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:82: 
The old code used to DCHECK the success of PostTaskAndReply.  Did we want to
replicate that here?

[Jói, 2011-10-11 19:51:02]

Thanks for the review.  I uploaded a new version that addresses your comments.

Cheers,
Jói

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:63: scoped_refptr<DhcpQuery>
dhcp_query(ImplCreateDhcpQuery());
On 2011/10/07 17:53:22, awong wrote:
> I wonder if it would be possible to not bother with the refcount here if you
> delete dhcp_query on the reply.
> 
> Might make your unittest's lifetime management slightly harder though.

I think if this way works, it seems more correct?  And yes might complicate unit
testing.

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:68: dhcp_query.get(),
On 2011/10/07 17:53:22, awong wrote:
> FYI, You no longer need the .get().  Submitted CL yesterday to fix.

Done.

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/8139028/diff/12001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:82: 
On 2011/10/07 17:53:22, awong wrote:
> The old code used to DCHECK the success of PostTaskAndReply.  Did we want to
> replicate that here?

Done.

[awong, 2011-10-12 08:21:23]

LGTM

[willchan no longer on Chromium, 2011-10-12 16:10:40]

On 2011/10/12 08:21:23, awong wrote:
> LGTM

I'll take a look today. Ping me if I forget.

[Jói, 2011-10-13 18:16:44]

Will: Friendly ping.  Beware my viking wrath, for I am less than 50
feet from you... ;-)


On Wed, Oct 12, 2011 at 9:10 AM,  <willchan@chromium.org> wrote:
> On 2011/10/12 08:21:23, awong wrote:
>>
>> LGTM
>
> I'll take a look today. Ping me if I forget.
>
> http://codereview.chromium.org/8139028/
>

[willchan no longer on Chromium, 2011-10-13 19:36:38]

I was hoping to get to this before lunch, but I'm too hungry. Eating first.

http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_an...
File base/threading/post_task_and_reply_impl.h (right):

http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_an...
base/threading/post_task_and_reply_impl.h:35: protected:
Why is this protected instead of private? Is a subclass calling this somewhere?

[Jói, 2011-10-13 20:27:58]

http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_an...
File base/threading/post_task_and_reply_impl.h (right):

http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_an...
base/threading/post_task_and_reply_impl.h:35: protected:
On 2011/10/13 19:36:38, willchan wrote:
> Why is this protected instead of private? Is a subclass calling this
somewhere?

Subclasses override this.

[willchan no longer on Chromium, 2011-10-13 20:30:02]

On Thu, Oct 13, 2011 at 1:27 PM, <joi@chromium.org> wrote:

>
> http://codereview.chromium.**org/8139028/diff/33001/base/**
>
threading/post_task_and_reply_**impl.h<http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_and_reply_impl.h>
> File base/threading/post_task_and_**reply_impl.h (right):
>
> http://codereview.chromium.**org/8139028/diff/33001/base/**
>
threading/post_task_and_reply_**impl.h#newcode35<http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_and_reply_impl.h#newcode35>
> base/threading/post_task_and_**reply_impl.h:35: protected:
> On 2011/10/13 19:36:38, willchan wrote:
>
>> Why is this protected instead of private? Is a subclass calling this
>>
> somewhere?
>
> Subclasses override this.
>

http://www.parashift.com/c++-faq-lite/strange-inheritance.html#faq-23.4

If subclasses need to call it, then virtual protected makes sense. If it
just needs to be overridden, then virtual private works great.


>
>
http://codereview.chromium.**org/8139028/<http://codereview.chromium.org/8139...
>

[willchan no longer on Chromium, 2011-10-13 22:33:45]

http://codereview.chromium.org/8139028/diff/33001/base/message_loop_proxy.cc
File base/message_loop_proxy.cc (right):

http://codereview.chromium.org/8139028/diff/33001/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:22: bool PostTask(const tracked_objects::Location&
from_here,
Please use the virtual keyword for this guy (style guide requires this). And
OVERRIDE.

http://codereview.chromium.org/8139028/diff/33001/base/threading/worker_pool.cc
File base/threading/worker_pool.cc (right):

http://codereview.chromium.org/8139028/diff/33001/base/threading/worker_pool....
base/threading/worker_pool.cc:20: bool PostTask(const tracked_objects::Location&
from_here,
virtual and OVERRIDE

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:125: std::string url() const;
Any reason not to make this return const std::string&?

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:125: last_query_.release();
Did you really mean release()? Not reset()? Is there a manual Release() call
somewhere to balance the AddRef() from the scoped_refptr you're release()ing?

[Jói, 2011-10-13 22:46:07]

PTAL, and thanks for a careful review.

http://codereview.chromium.org/8139028/diff/33001/base/message_loop_proxy.cc
File base/message_loop_proxy.cc (right):

http://codereview.chromium.org/8139028/diff/33001/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:22: bool PostTask(const tracked_objects::Location&
from_here,
On 2011/10/13 22:33:45, willchan wrote:
> Please use the virtual keyword for this guy (style guide requires this). And
> OVERRIDE.

Done.

http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_an...
File base/threading/post_task_and_reply_impl.h (right):

http://codereview.chromium.org/8139028/diff/33001/base/threading/post_task_an...
base/threading/post_task_and_reply_impl.h:35: protected:
On 2011/10/13 19:36:38, willchan wrote:
> Why is this protected instead of private? Is a subclass calling this
somewhere?

Made it private, it is only overridden.

http://codereview.chromium.org/8139028/diff/33001/base/threading/worker_pool.cc
File base/threading/worker_pool.cc (right):

http://codereview.chromium.org/8139028/diff/33001/base/threading/worker_pool....
base/threading/worker_pool.cc:20: bool PostTask(const tracked_objects::Location&
from_here,
On 2011/10/13 22:33:45, willchan wrote:
> virtual and OVERRIDE

Done.

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:125: std::string url() const;
On 2011/10/13 22:33:45, willchan wrote:
> Any reason not to make this return const std::string&?

Good catch, fixed.

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/8139028/diff/33001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:125: last_query_.release();
On 2011/10/13 22:33:45, willchan wrote:
> Did you really mean release()? Not reset()? Is there a manual Release() call
> somewhere to balance the AddRef() from the scoped_refptr you're release()ing?

Argh, that's a bad method name, thanks for catching.  Changed to last_query_ =
NULL (there is no reset()).

[willchan no longer on Chromium, 2011-10-13 22:59:56]

lgtm

http://codereview.chromium.org/8139028/diff/33002/base/message_loop_proxy.cc
File base/message_loop_proxy.cc (right):

http://codereview.chromium.org/8139028/diff/33002/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:22: protected:
private

http://codereview.chromium.org/8139028/diff/33002/base/threading/worker_pool.cc
File base/threading/worker_pool.cc (right):

http://codereview.chromium.org/8139028/diff/33002/base/threading/worker_pool....
base/threading/worker_pool.cc:21: virtual bool PostTask(const
tracked_objects::Location& from_here,
private

[Jói, 2011-10-13 23:02:49]

http://codereview.chromium.org/8139028/diff/33002/base/message_loop_proxy.cc
File base/message_loop_proxy.cc (right):

http://codereview.chromium.org/8139028/diff/33002/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:22: protected:
On 2011/10/13 22:59:56, willchan wrote:
> private

Done.

http://codereview.chromium.org/8139028/diff/33002/base/threading/worker_pool.cc
File base/threading/worker_pool.cc (right):

http://codereview.chromium.org/8139028/diff/33002/base/threading/worker_pool....
base/threading/worker_pool.cc:21: virtual bool PostTask(const
tracked_objects::Location& from_here,
On 2011/10/13 22:59:56, willchan wrote:
> private

Done.