Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7210053:
Implementation of PostTaskAndReply() in MessageLoopProxy and BrowserThread. (Closed)
|
Created:
9 years, 5 months ago by awong Modified:
9 years, 4 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Dai Mikurube (NOT FULLTIME) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementation of PostTaskAndReply() in MessageLoopProxy and BrowserThread.
This ensures that the request/reply closures are always deleted on the origin
thread, or leaked if the task cannot be completed (due to message loop
shutdown).
BUG=86301
TEST=new unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97387
Patch Set 1 #
Total comments: 15
Patch Set 2 : Real implementation #Patch Set 3 : missed something small #
Total comments: 4
Patch Set 4 : rebased and turned into methods #
Total comments: 4
Patch Set 5 : Added unittests #
Total comments: 6
Patch Set 6 : Move PTAR to message_loop_proxy.h and add more unittests. #Patch Set 7 : revert useless changes #Patch Set 8 : iwyu #
Total comments: 8
Patch Set 9 : Address Darin's comments. #Patch Set 10 : don't hang browserthread. #
Total comments: 4
Patch Set 11 : address Darin's latest comments. #Patch Set 12 : upload the correct version #Patch Set 13 : add supression #
Total comments: 5
Patch Set 14 : add in a bug #Patch Set 15 : copyright #
Messages
Total messages: 32 (0 generated)
This is a (nearly) simplest-possible implementation. Darin, would this be useful enough for you to test with? Or is being able to pass the return value from the task into the argument for the reply critical? I can do that too...but it'll need more templates. This CL hasn't been tested yet...so just looking for conceptual feedback. Then I'll flush out tests and make this thing work. -Albert
Yeah, this looks like a good approach. http://codereview.chromium.org/7210053/diff/1/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode54 base/task.cc:54: origin_loop_->PostTask( what if the origin thread is already gone? i suspect you need to use a MessageLoopProxy instead. http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode63 base/task.cc:63: } // namespace internal nit: add a new line above the close of the namespace http://codereview.chromium.org/7210053/diff/1/base/task.h File base/task.h (right): http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode584 base/task.h:584: } // namespace internal nit: add a new line above the close of the namespace http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode586 base/task.h:586: template <typename MessageLoopType, typename ResponseType> I guess ResponseType is just a placeholder.
http://codereview.chromium.org/7210053/diff/1/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode54 base/task.cc:54: origin_loop_->PostTask( On 2011/07/01 04:15:32, darin wrote: > what if the origin thread is already gone? i suspect you need to use a > MessageLoopProxy instead. Yes, MessageLoopProxy FTW. http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode55 base/task.cc:55: base::Bind(&PostTaskAndReplyRelay::RunReplyAndSelfDestruct, We're already in base::, so the prefix probably isn't necessary. http://codereview.chromium.org/7210053/diff/1/base/task.h File base/task.h (right): http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode587 base/task.h:587: void PostTaskAndReply(MessageLoopType loop, const Closure& task, Why is MessageLoopType templated? Is this to make it work for WorkerPool also? Note that there is no WorkerPool instance right now, it's a class static API. That'll have to change for this to work. I guess I had originally imagined we'd use an abstract TaskRunner interface, but I guess templating works too, since we're only using one method. I'd advocate for naming the template parameter as TaskRunner or Executor. http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode590 base/task.h:590: loop.PostTask(&PostTaskAndReplyRelay::Run, base::Unretained(relay)); Does this code actually work? Is there no need for FROM_HERE?
Okay, here is draft 2. The main differences are: 1) no templates 2) readding refcounting to the relay. I still need a unittest. Also, I need another CL that adds Closure support to MessageLoop before this can go in. http://codereview.chromium.org/7210053/diff/1/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode54 base/task.cc:54: origin_loop_->PostTask( On 2011/07/01 04:15:32, darin wrote: > what if the origin thread is already gone? i suspect you need to use a > MessageLoopProxy instead. Done. http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode55 base/task.cc:55: base::Bind(&PostTaskAndReplyRelay::RunReplyAndSelfDestruct, On 2011/07/01 15:13:54, willchan wrote: > We're already in base::, so the prefix probably isn't necessary. Done. http://codereview.chromium.org/7210053/diff/1/base/task.cc#newcode63 base/task.cc:63: } // namespace internal On 2011/07/01 04:15:32, darin wrote: > nit: add a new line above the close of the namespace Done. http://codereview.chromium.org/7210053/diff/1/base/task.h File base/task.h (right): http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode584 base/task.h:584: } // namespace internal On 2011/07/01 04:15:32, darin wrote: > nit: add a new line above the close of the namespace Done. http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode586 base/task.h:586: template <typename MessageLoopType, typename ResponseType> On 2011/07/01 04:15:32, darin wrote: > I guess ResponseType is just a placeholder. No...just a mistake due to rushing this CL out. Removed. http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode587 base/task.h:587: void PostTaskAndReply(MessageLoopType loop, const Closure& task, On 2011/07/01 15:13:54, willchan wrote: > Why is MessageLoopType templated? Is this to make it work for WorkerPool also? > Note that there is no WorkerPool instance right now, it's a class static API. > That'll have to change for this to work. > > I guess I had originally imagined we'd use an abstract TaskRunner interface, but > I guess templating works too, since we're only using one method. > > I'd advocate for naming the template parameter as TaskRunner or Executor. Actually, I was trying bridge between MessageLoopProxy and MessageLoop, but just got it completely wrong. It's irksome that syntactically BrowserThread, MessageLoop, and MessageLoopProxy all have slightly different PostTask semantics. But I don't think there's a way to unify. In the long term, I think that each class should provide syntactic sugar for this function. I'm going to reduce this to just one function, no templates or overloads, and then we can add syntactic sugar to each of BrowserThread, MessageLoop, MessageLoopProxy, WorkerPool, etc., as wanted. http://codereview.chromium.org/7210053/diff/1/base/task.h#newcode590 base/task.h:590: loop.PostTask(&PostTaskAndReplyRelay::Run, base::Unretained(relay)); On 2011/07/01 15:13:54, willchan wrote: > Does this code actually work? Is there no need for FROM_HERE? Nope...doesn't work yet. Will soon.
On Fri, Jul 1, 2011 at 5:36 PM, <ajwong@chromium.org> wrote: > Okay, here is draft 2. > > The main differences are: > 1) no templates > 2) readding refcounting to the relay. > What is the reason for adding refcounting to the relay? > > I still need a unittest. Also, I need another CL that adds Closure support > to > MessageLoop before this can go in. > > > > http://codereview.chromium.**org/7210053/diff/1/base/task.**cc<http://coderev... > File base/task.cc (right): > > http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode54<http... > base/task.cc:54: origin_loop_->PostTask( > On 2011/07/01 04:15:32, darin wrote: > >> what if the origin thread is already gone? i suspect you need to use >> > a > >> MessageLoopProxy instead. >> > > Done. > > > http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode55<http... > base/task.cc:55: > base::Bind(&**PostTaskAndReplyRelay::**RunReplyAndSelfDestruct, > On 2011/07/01 15:13:54, willchan wrote: > >> We're already in base::, so the prefix probably isn't necessary. >> > > Done. > > > http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode63<http... > base/task.cc:63: } // namespace internal > On 2011/07/01 04:15:32, darin wrote: > >> nit: add a new line above the close of the namespace >> > > Done. > > > http://codereview.chromium.**org/7210053/diff/1/base/task.h<http://codereview... > File base/task.h (right): > > http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode584<http... > base/task.h:584: } // namespace internal > On 2011/07/01 04:15:32, darin wrote: > >> nit: add a new line above the close of the namespace >> > > Done. > > > http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode586<http... > base/task.h:586: template <typename MessageLoopType, typename > ResponseType> > On 2011/07/01 04:15:32, darin wrote: > >> I guess ResponseType is just a placeholder. >> > > No...just a mistake due to rushing this CL out. Removed. > > > http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode587<http... > base/task.h:587: void PostTaskAndReply(**MessageLoopType loop, const > Closure& task, > On 2011/07/01 15:13:54, willchan wrote: > >> Why is MessageLoopType templated? Is this to make it work for >> > WorkerPool also? > >> Note that there is no WorkerPool instance right now, it's a class >> > static API. > >> That'll have to change for this to work. >> > > I guess I had originally imagined we'd use an abstract TaskRunner >> > interface, but > >> I guess templating works too, since we're only using one method. >> > > I'd advocate for naming the template parameter as TaskRunner or >> > Executor. > > Actually, I was trying bridge between MessageLoopProxy and MessageLoop, > but just got it completely wrong. > > It's irksome that syntactically BrowserThread, MessageLoop, and > MessageLoopProxy all have slightly different PostTask semantics. But I > don't think there's a way to unify. In the long term, I think that each > class should provide syntactic sugar for this function. > > I'm going to reduce this to just one function, no templates or > overloads, and then we can add syntactic sugar to each of BrowserThread, > MessageLoop, MessageLoopProxy, WorkerPool, etc., as wanted. > > > http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode590<http... > base/task.h:590: loop.PostTask(&**PostTaskAndReplyRelay::Run, > base::Unretained(relay)); > On 2011/07/01 15:13:54, willchan wrote: > >> Does this code actually work? Is there no need for FROM_HERE? >> > > Nope...doesn't work yet. Will soon. > > > http://codereview.chromium.**org/7210053/<http://codereview.chromium.org/7210... >
On Wed, Jul 6, 2011 at 12:46 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Fri, Jul 1, 2011 at 5:36 PM, <ajwong@chromium.org> wrote: > >> Okay, here is draft 2. >> >> The main differences are: >> 1) no templates >> 2) readding refcounting to the relay. >> > > What is the reason for adding refcounting to the relay? > See the comment in task.h, like 592. Basically, if I post to a MessageLoop that has already quit, but the originating loop is still alive, the refcounting allows deletion of the relay. Maybe this is too much of an edge case to care about? -Albert > >> I still need a unittest. Also, I need another CL that adds Closure >> support to >> MessageLoop before this can go in. >> >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc<http://coderev... >> File base/task.cc (right): >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode54<http... >> base/task.cc:54: origin_loop_->PostTask( >> On 2011/07/01 04:15:32, darin wrote: >> >>> what if the origin thread is already gone? i suspect you need to use >>> >> a >> >>> MessageLoopProxy instead. >>> >> >> Done. >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode55<http... >> base/task.cc:55: >> base::Bind(&**PostTaskAndReplyRelay::**RunReplyAndSelfDestruct, >> On 2011/07/01 15:13:54, willchan wrote: >> >>> We're already in base::, so the prefix probably isn't necessary. >>> >> >> Done. >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode63<http... >> base/task.cc:63: } // namespace internal >> On 2011/07/01 04:15:32, darin wrote: >> >>> nit: add a new line above the close of the namespace >>> >> >> Done. >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.h<http://codereview... >> File base/task.h (right): >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode584<http... >> base/task.h:584: } // namespace internal >> On 2011/07/01 04:15:32, darin wrote: >> >>> nit: add a new line above the close of the namespace >>> >> >> Done. >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode586<http... >> base/task.h:586: template <typename MessageLoopType, typename >> ResponseType> >> On 2011/07/01 04:15:32, darin wrote: >> >>> I guess ResponseType is just a placeholder. >>> >> >> No...just a mistake due to rushing this CL out. Removed. >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode587<http... >> base/task.h:587: void PostTaskAndReply(**MessageLoopType loop, const >> Closure& task, >> On 2011/07/01 15:13:54, willchan wrote: >> >>> Why is MessageLoopType templated? Is this to make it work for >>> >> WorkerPool also? >> >>> Note that there is no WorkerPool instance right now, it's a class >>> >> static API. >> >>> That'll have to change for this to work. >>> >> >> I guess I had originally imagined we'd use an abstract TaskRunner >>> >> interface, but >> >>> I guess templating works too, since we're only using one method. >>> >> >> I'd advocate for naming the template parameter as TaskRunner or >>> >> Executor. >> >> Actually, I was trying bridge between MessageLoopProxy and MessageLoop, >> but just got it completely wrong. >> >> It's irksome that syntactically BrowserThread, MessageLoop, and >> MessageLoopProxy all have slightly different PostTask semantics. But I >> don't think there's a way to unify. In the long term, I think that each >> class should provide syntactic sugar for this function. >> >> I'm going to reduce this to just one function, no templates or >> overloads, and then we can add syntactic sugar to each of BrowserThread, >> MessageLoop, MessageLoopProxy, WorkerPool, etc., as wanted. >> >> >> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode590<http... >> base/task.h:590: loop.PostTask(&**PostTaskAndReplyRelay::Run, >> base::Unretained(relay)); >> On 2011/07/01 15:13:54, willchan wrote: >> >>> Does this code actually work? Is there no need for FROM_HERE? >>> >> >> Nope...doesn't work yet. Will soon. >> >> >> http://codereview.chromium.**org/7210053/<http://codereview.chromium.org/7210... >> > >
On Wed, Jul 6, 2011 at 12:49 PM, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote: > > > On Wed, Jul 6, 2011 at 12:46 PM, Darin Fisher <darin@chromium.org> wrote: > >> >> >> On Fri, Jul 1, 2011 at 5:36 PM, <ajwong@chromium.org> wrote: >> >>> Okay, here is draft 2. >>> >>> The main differences are: >>> 1) no templates >>> 2) readding refcounting to the relay. >>> >> >> What is the reason for adding refcounting to the relay? >> > > See the comment in task.h, like 592. > > Basically, if I post to a MessageLoop that has already quit, but the > originating loop is still alive, the refcounting allows deletion of the > relay. > > Maybe this is too much of an edge case to care about? > > Yeah, that's what I'm wondering. I know I created the ManualDestruction doodad to support this use case, but I was sort of bugged by how that adds complexity. I think you are trying to avoid leaking the relay, but that might be unavoidable in some cases anyways. I think that PostTaskAndReply should also not be a global function. It should be a member function of MessageLoop, MessageLoopProxy, and BrowserThread. (BrowserThread support could come later, but it is probably easy to add now.) It is also an interesting idea to consider not adding this to MessageLoop. That would encourage use of MessageLoopProxy and BrowserThread, which is probably a good thing. Having PostTaskAndReplyRelay in task.h is probably good as it enables sharing the code between MessageLoopProxy and BrowserThread. Otherwise, I would just hide that class in a .cc file. -Darin > -Albert > > > >> >>> I still need a unittest. Also, I need another CL that adds Closure >>> support to >>> MessageLoop before this can go in. >>> >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc<http://coderev... >>> File base/task.cc (right): >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode54<http... >>> base/task.cc:54: origin_loop_->PostTask( >>> On 2011/07/01 04:15:32, darin wrote: >>> >>>> what if the origin thread is already gone? i suspect you need to use >>>> >>> a >>> >>>> MessageLoopProxy instead. >>>> >>> >>> Done. >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode55<http... >>> base/task.cc:55: >>> base::Bind(&**PostTaskAndReplyRelay::**RunReplyAndSelfDestruct, >>> On 2011/07/01 15:13:54, willchan wrote: >>> >>>> We're already in base::, so the prefix probably isn't necessary. >>>> >>> >>> Done. >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**cc#newcode63<http... >>> base/task.cc:63: } // namespace internal >>> On 2011/07/01 04:15:32, darin wrote: >>> >>>> nit: add a new line above the close of the namespace >>>> >>> >>> Done. >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.h<http://codereview... >>> File base/task.h (right): >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode584<http... >>> base/task.h:584: } // namespace internal >>> On 2011/07/01 04:15:32, darin wrote: >>> >>>> nit: add a new line above the close of the namespace >>>> >>> >>> Done. >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode586<http... >>> base/task.h:586: template <typename MessageLoopType, typename >>> ResponseType> >>> On 2011/07/01 04:15:32, darin wrote: >>> >>>> I guess ResponseType is just a placeholder. >>>> >>> >>> No...just a mistake due to rushing this CL out. Removed. >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode587<http... >>> base/task.h:587: void PostTaskAndReply(**MessageLoopType loop, const >>> Closure& task, >>> On 2011/07/01 15:13:54, willchan wrote: >>> >>>> Why is MessageLoopType templated? Is this to make it work for >>>> >>> WorkerPool also? >>> >>>> Note that there is no WorkerPool instance right now, it's a class >>>> >>> static API. >>> >>>> That'll have to change for this to work. >>>> >>> >>> I guess I had originally imagined we'd use an abstract TaskRunner >>>> >>> interface, but >>> >>>> I guess templating works too, since we're only using one method. >>>> >>> >>> I'd advocate for naming the template parameter as TaskRunner or >>>> >>> Executor. >>> >>> Actually, I was trying bridge between MessageLoopProxy and MessageLoop, >>> but just got it completely wrong. >>> >>> It's irksome that syntactically BrowserThread, MessageLoop, and >>> MessageLoopProxy all have slightly different PostTask semantics. But I >>> don't think there's a way to unify. In the long term, I think that each >>> class should provide syntactic sugar for this function. >>> >>> I'm going to reduce this to just one function, no templates or >>> overloads, and then we can add syntactic sugar to each of BrowserThread, >>> MessageLoop, MessageLoopProxy, WorkerPool, etc., as wanted. >>> >>> >>> http://codereview.chromium.**org/7210053/diff/1/base/task.**h#newcode590<http... >>> base/task.h:590: loop.PostTask(&**PostTaskAndReplyRelay::Run, >>> base::Unretained(relay)); >>> On 2011/07/01 15:13:54, willchan wrote: >>> >>>> Does this code actually work? Is there no need for FROM_HERE? >>>> >>> >>> Nope...doesn't work yet. Will soon. >>> >>> >>> http://codereview.chromium.**org/7210053/<http://codereview.chromium.org/7210... >>> >> >> >
I don't know that I have any opinion on whether this needs to be a member function or a free function. http://codereview.chromium.org/7210053/diff/4002/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/4002/base/task.cc#newcode85 base/task.cc:85: void PostTaskAndReply(MessageLoop* loop, We should use MessageLoopProxy here. http://codereview.chromium.org/7210053/diff/4002/base/task.h File base/task.h (right): http://codereview.chromium.org/7210053/diff/4002/base/task.h#newcode595 base/task.h:595: // target MessageLoop is in shutdown, but the origin MessageLoop is not. I think this is unnecessarily complicated and this does not buy us anything. If |task| does not run, then I think the behavior is undefined anyway. Allowing |task| to be destroyed (the destructor of a Closure may have behavior) even when not Run() may cause its own problems (it's possible destroying the Closure may release the last reference which invokes a destructor which expects that the task has run, and if not, crashes). If you need defined behavior in the presence of shutdown, you need to code something up on your own.
I'll remove the refcount, and move it to a member function on the various classes. I'll come back to this once I get Closure support woven into everything (http://codereview.chromium.org/7316015/) On 2011/07/06 22:10:26, willchan wrote: > I don't know that I have any opinion on whether this needs to be a member > function or a free function. > > http://codereview.chromium.org/7210053/diff/4002/base/task.cc > File base/task.cc (right): > > http://codereview.chromium.org/7210053/diff/4002/base/task.cc#newcode85 > base/task.cc:85: void PostTaskAndReply(MessageLoop* loop, > We should use MessageLoopProxy here. > > http://codereview.chromium.org/7210053/diff/4002/base/task.h > File base/task.h (right): > > http://codereview.chromium.org/7210053/diff/4002/base/task.h#newcode595 > base/task.h:595: // target MessageLoop is in shutdown, but the origin > MessageLoop is not. > I think this is unnecessarily complicated and this does not buy us anything. If > |task| does not run, then I think the behavior is undefined anyway. Allowing > |task| to be destroyed (the destructor of a Closure may have behavior) even when > not Run() may cause its own problems (it's possible destroying the Closure may > release the last reference which invokes a destructor which expects that the > task has run, and if not, crashes). If you need defined behavior in the presence > of shutdown, you need to code something up on your own.
PTAL. Still need unittest. http://codereview.chromium.org/7210053/diff/4002/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/4002/base/task.cc#newcode85 base/task.cc:85: void PostTaskAndReply(MessageLoop* loop, On 2011/07/06 22:10:26, willchan wrote: > We should use MessageLoopProxy here. New CL side-steps this issue. http://codereview.chromium.org/7210053/diff/4002/base/task.h File base/task.h (right): http://codereview.chromium.org/7210053/diff/4002/base/task.h#newcode595 base/task.h:595: // target MessageLoop is in shutdown, but the origin MessageLoop is not. On 2011/07/06 22:10:26, willchan wrote: > I think this is unnecessarily complicated and this does not buy us anything. If > |task| does not run, then I think the behavior is undefined anyway. Allowing > |task| to be destroyed (the destructor of a Closure may have behavior) even when > not Run() may cause its own problems (it's possible destroying the Closure may > release the last reference which invokes a destructor which expects that the > task has run, and if not, crashes). If you need defined behavior in the presence > of shutdown, you need to code something up on your own. Okay, went back to simple delete.
things are looking fine to me, would love to see tests http://codereview.chromium.org/7210053/diff/20012/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/20012/base/task.cc#newcode93 base/task.cc:93: void PostTaskAndReplyRelay::RunReplyAndSelfDestruct() { Should |task_| be set to NULL at this point, which would free any bound parameters? I also note that due to the member ordering, task_'s bound parameters will be freed after reply_'s. I need to think about it more, but my instinct is that that's undesirable.
http://codereview.chromium.org/7210053/diff/20012/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/20012/base/task.cc#newcode93 base/task.cc:93: void PostTaskAndReplyRelay::RunReplyAndSelfDestruct() { On 2011/08/16 02:00:12, willchan wrote: > Should |task_| be set to NULL at this point, which would free any bound > parameters? I also note that due to the member ordering, task_'s bound > parameters will be freed after reply_'s. I need to think about it more, but my > instinct is that that's undesirable. I think the delete this is good enough to force the destruction of |task_| and |reply_|. Good point about the ordering though. It'd be nice to define an ordering in the API contract. I agree that reply feels like it should be destroyed first. Will make that change tomorrow.
http://codereview.chromium.org/7210053/diff/20012/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/20012/base/task.cc#newcode93 base/task.cc:93: void PostTaskAndReplyRelay::RunReplyAndSelfDestruct() { On 2011/08/16 02:12:13, awong wrote: > On 2011/08/16 02:00:12, willchan wrote: > > Should |task_| be set to NULL at this point, which would free any bound > > parameters? I also note that due to the member ordering, task_'s bound > > parameters will be freed after reply_'s. I need to think about it more, but my > > instinct is that that's undesirable. > > I think the delete this is good enough to force the destruction of |task_| and > |reply_|. I won't push further, but I just wonder if there's ever a case where reply_ may have expected that task_ should already have been destroyed. It's possible that there's behavior in the destructor of a bound argument. The API needs to dictate the lifetime expectations. My instinct again was that |task_|'s bound arguments should be destroyed prior to the execution of |reply_|. I haven't thought about it too much though. > > Good point about the ordering though. It'd be nice to define an ordering in the > API contract. I agree that reply feels like it should be destroyed first. Will > make that change tomorrow.
Unittest added. Please let me know what you think. FYI, the unittest introduces an intentional memory leak. I can't think of a way around this because the relay code DCHECKs that it's on the right MessageLoop on deletion. Should I just file a valgrind suppression? http://codereview.chromium.org/7210053/diff/20012/base/task.cc File base/task.cc (right): http://codereview.chromium.org/7210053/diff/20012/base/task.cc#newcode93 base/task.cc:93: void PostTaskAndReplyRelay::RunReplyAndSelfDestruct() { On 2011/08/16 14:50:56, willchan wrote: > On 2011/08/16 02:12:13, awong wrote: > > On 2011/08/16 02:00:12, willchan wrote: > > > Should |task_| be set to NULL at this point, which would free any bound > > > parameters? I also note that due to the member ordering, task_'s bound > > > parameters will be freed after reply_'s. I need to think about it more, but > my > > > instinct is that that's undesirable. > > > > I think the delete this is good enough to force the destruction of |task_| and > > |reply_|. > > I won't push further, but I just wonder if there's ever a case where reply_ may > have expected that task_ should already have been destroyed. It's possible that > there's behavior in the destructor of a bound argument. The API needs to dictate > the lifetime expectations. My instinct again was that |task_|'s bound arguments > should be destroyed prior to the execution of |reply_|. I haven't thought about > it too much though. > > > > > Good point about the ordering though. It'd be nice to define an ordering in > the > > API contract. I agree that reply feels like it should be destroyed first. > Will > > make that change tomorrow. > AH! I see what you're saying now. Yes. I can see that being somewhat useful in ensuring people don't depend that the bindings in task outlive reply. I've changed the code to force this, and documented the contract in the API.
What do you think about adding a test that enforces the destruction order of task and reply? Perhaps the reply closure should check to make sure that task has been destroyed? I wonder if it'd be better to test through one of the public interfaces (MessageLoopProxy for example). I try to avoid testing the internals unless it's necessary to get direct access (maybe it is?). If you could address my comments tomorrow morning, I'd appreciate that. I'd like to send the approval before going to India (I fly out in the afternoon). http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc File base/task_unittest.cc (right): http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc#newco... base/task_unittest.cc:114: class LoopRecorder : public base::RefCountedThreadSafe<LoopRecorder> { I'm not sure why you make this RefCountedThreadSafe instead of just making this a stack allocated variable and use base::Unretained(). Maybe that's my anti-refcounting bias :P http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc#newco... base/task_unittest.cc:154: scoped_refptr<LoopRecorder> task_recoder = s/task_recoder/task_recorder/? http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc#newco... base/task_unittest.cc:275: // TODO(ajwong): Is there a way to make Valgrind not hate us? delete relay; :)
I can try to add something that checks destruction order tomorrow. http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc File base/task_unittest.cc (right): http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc#newco... base/task_unittest.cc:114: class LoopRecorder : public base::RefCountedThreadSafe<LoopRecorder> { On 2011/08/17 04:36:07, willchan wrote: > I'm not sure why you make this RefCountedThreadSafe instead of just making this > a stack allocated variable and use base::Unretained(). Maybe that's my > anti-refcounting bias :P I figured that in the case that someone introduced a bug where it was deleted on the wrong thread, this would at least ensure defined behavior. As for why refcounted, I'm trying to detect when the Closure itself is deleted so I need some sort of refcount in the bound argument. http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc#newco... base/task_unittest.cc:154: scoped_refptr<LoopRecorder> task_recoder = On 2011/08/17 04:36:07, willchan wrote: > s/task_recoder/task_recorder/? Will fix tomorrow. http://codereview.chromium.org/7210053/diff/29001/base/task_unittest.cc#newco... base/task_unittest.cc:275: // TODO(ajwong): Is there a way to make Valgrind not hate us? On 2011/08/17 04:36:07, willchan wrote: > delete relay; :) Tried that. Dies cause the Relay asserts that it is being deleted on the correct thread. I could remove that dcheck, but leaking seemed better. :(
Beefed up the tests a bit more and moved some files around. Also fixed an unnecessary leak in MessageLoopProxy's PostTaskAndReply. Let me know what you think. (Btw, threading is hard. Just as observation. :-/)
What's the plan for using PostTaskAndReply to compute a result on a background thread and process the computed result back on the origin thread? http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc File base/message_loop_proxy.cc (right): http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#... base/message_loop_proxy.cc:24: base::Unretained(relay)))) { nit: indent by 4 fewer characters? since this is the second argument of the base::Bind function call, i'd expect the arguments to be left-aligned in the same column. the current indenting makes it look like a continuation of the first argument or something. nit: no need for base:: prefix. http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#... base/message_loop_proxy.cc:57: from_here_, this could make the data in about:objects look confusing. maybe we should somehow append a suffix to from_here_ so that it will be identified as a the reply task? http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#... base/message_loop_proxy.cc:64: task_.Reset(); it seems like there might be some subtle, non-trivial reason for resetting this task before invoking the reply callback. maybe you should insert a comment here explaining that reason? http://codereview.chromium.org/7210053/diff/35012/content/browser/browser_thr... File content/browser/browser_thread.cc (right): http://codereview.chromium.org/7210053/diff/35012/content/browser/browser_thr... content/browser/browser_thread.cc:234: base::Bind(&PostTaskAndReplyRelay::Run, It almost seems like there could be room for a static function defined within the base:: namespace that takes (from_here, task, reply) and returns a "PostTaskAndReplyRelay" base::Closure. This way you don't need to replicate this somewhat subtle setup and invocation code for using PostTaskAndReplyRelay. Plus, using the base::internal:: namespace outside of base/ is just wrong. We should have a presubmit check that no one does this. return PostTask(identifier, from_here, base::CreatePostTaskAndReplyRelay(from_here, task, reply)); We could probably come up with an even better name than CreatePostTaskAndReplyRelay.
As is, the framework does not pass the return value along. The expectation is
the user passes the context via currying. For example.
class Foo {
public:
void Do() {
proxy->PostTaskAndReply(FROM_HERE,
Bind(&Compute, &my_result),
Bind(&Process, this));
}
void Process {
};
private:
int my_result;
};
http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc
File base/message_loop_proxy.cc (right):
http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:24: base::Unretained(relay)))) {
On 2011/08/17 17:20:39, darin wrote:
> nit: indent by 4 fewer characters? since this is the second argument of the
> base::Bind function call, i'd expect the arguments to be left-aligned in the
> same column. the current indenting makes it look like a continuation of the
> first argument or something.
>
> nit: no need for base:: prefix.
Done.
http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:57: from_here_,
On 2011/08/17 17:20:39, darin wrote:
> this could make the data in about:objects look confusing. maybe we should
> somehow append a suffix to from_here_ so that it will be identified as a the
> reply task?
Do you think so? As is, the "FROM_HERE" is going to be from a PostTaskAndReply
call, which should be the point of interest right?
http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#...
base/message_loop_proxy.cc:64: task_.Reset();
On 2011/08/17 17:20:39, darin wrote:
> it seems like there might be some subtle, non-trivial reason for resetting
this
> task before invoking the reply callback. maybe you should insert a comment
here
> explaining that reason?
Done.
http://codereview.chromium.org/7210053/diff/35012/content/browser/browser_thr...
File content/browser/browser_thread.cc (right):
http://codereview.chromium.org/7210053/diff/35012/content/browser/browser_thr...
content/browser/browser_thread.cc:234: base::Bind(&PostTaskAndReplyRelay::Run,
On 2011/08/17 17:20:39, darin wrote:
> It almost seems like there could be room for a static function defined within
> the base:: namespace that takes (from_here, task, reply) and returns a
> "PostTaskAndReplyRelay" base::Closure. This way you don't need to replicate
> this somewhat subtle setup and invocation code for using
PostTaskAndReplyRelay.
>
> Plus, using the base::internal:: namespace outside of base/ is just wrong. We
> should have a presubmit check that no one does this.
>
> return PostTask(identifier, from_here,
> base::CreatePostTaskAndReplyRelay(from_here, task, reply));
>
> We could probably come up with an even better name than
> CreatePostTaskAndReplyRelay.
Good point...especially considering I *just* made a mistake with this little
fork. Grr.
I changed the code to call the version on MessageLoopProxy. Trying to extract a
function out of this is ridiculously painful cause you have to ensure that
1) relay is new-ed on the right thread
2) you can delete relay on a failure.
On Wed, Aug 17, 2011 at 11:46 AM, <ajwong@chromium.org> wrote: > As is, the framework does not pass the return value along. The expectation > is > the user passes the context via currying. For example. > > class Foo { > public: > void Do() { > proxy->PostTaskAndReply(FROM_**HERE, > Bind(&Compute, &my_result), > Bind(&Process, this)); > } > void Process { > }; > private: > int my_result; > > }; > > Oh, I see... so my_result has to be safe to mutate from a background thread? It seems like a copying semantic may be the safer default, but your approach has a certain simplicity to it that is kinda nice. We should probably go with this and see if it leads to any troubles. > > http://codereview.chromium.**org/7210053/diff/35012/base/** > message_loop_proxy.cc<http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc> > File base/message_loop_proxy.cc (right): > > http://codereview.chromium.**org/7210053/diff/35012/base/** > message_loop_proxy.cc#**newcode24<http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#newcode24> > base/message_loop_proxy.cc:24: base::Unretained(relay)))) { > On 2011/08/17 17:20:39, darin wrote: > >> nit: indent by 4 fewer characters? since this is the second argument >> > of the > >> base::Bind function call, i'd expect the arguments to be left-aligned >> > in the > >> same column. the current indenting makes it look like a continuation >> > of the > >> first argument or something. >> > > nit: no need for base:: prefix. >> > > Done. > > > http://codereview.chromium.**org/7210053/diff/35012/base/** > message_loop_proxy.cc#**newcode57<http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#newcode57> > base/message_loop_proxy.cc:57: from_here_, > On 2011/08/17 17:20:39, darin wrote: > >> this could make the data in about:objects look confusing. maybe we >> > should > >> somehow append a suffix to from_here_ so that it will be identified as >> > a the > >> reply task? >> > > Do you think so? As is, the "FROM_HERE" is going to be from a > PostTaskAndReply call, which should be the point of interest right? Right, but... What I meant is that you will have two separate tasks combined together under the same value of from_here. If only the reply task is slow, then the numbers will be misleading. It would be nice to have the initial task and the reply task accounted for separately in about:objects. I think the name of the reply task should be derived from the user supplied from_here value. Maybe tack on a "(reply)" suffix. > > > http://codereview.chromium.**org/7210053/diff/35012/base/** > message_loop_proxy.cc#**newcode64<http://codereview.chromium.org/7210053/diff/35012/base/message_loop_proxy.cc#newcode64> > base/message_loop_proxy.cc:64: task_.Reset(); > On 2011/08/17 17:20:39, darin wrote: > >> it seems like there might be some subtle, non-trivial reason for >> > resetting this > >> task before invoking the reply callback. maybe you should insert a >> > comment here > >> explaining that reason? >> > > Done. > > > http://codereview.chromium.**org/7210053/diff/35012/** > content/browser/browser_**thread.cc<http://codereview.chromium.org/7210053/diff/35012/content/browser/browser_thread.cc> > File content/browser/browser_**thread.cc (right): > > http://codereview.chromium.**org/7210053/diff/35012/** > content/browser/browser_**thread.cc#newcode234<http://codereview.chromium.org/7210053/diff/35012/content/browser/browser_thread.cc#newcode234> > content/browser/browser_**thread.cc:234: > base::Bind(&**PostTaskAndReplyRelay::Run, > On 2011/08/17 17:20:39, darin wrote: > >> It almost seems like there could be room for a static function defined >> > within > >> the base:: namespace that takes (from_here, task, reply) and returns a >> "PostTaskAndReplyRelay" base::Closure. This way you don't need to >> > replicate > >> this somewhat subtle setup and invocation code for using >> > PostTaskAndReplyRelay. > > Plus, using the base::internal:: namespace outside of base/ is just >> > wrong. We > >> should have a presubmit check that no one does this. >> > > return PostTask(identifier, from_here, >> base::**CreatePostTaskAndReplyRelay(**from_here, task, >> > reply)); > > We could probably come up with an even better name than >> CreatePostTaskAndReplyRelay. >> > > Good point...especially considering I *just* made a mistake with this > little fork. Grr. > > I changed the code to call the version on MessageLoopProxy. Trying to > extract a function out of this is ridiculously painful cause you have to > ensure that > > 1) relay is new-ed on the right thread > 2) you can delete relay on a failure. > > OK > > http://codereview.chromium.**org/7210053/<http://codereview.chromium.org/7210... >
LGTM http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h File base/message_loop_proxy.h (right): http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h#n... base/message_loop_proxy.h:86: // while |task| may still be executing. it might help to have some examples here of how you use parameter currying to capture results. it may also be helpful to show examples using WeakPtr. http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h#n... base/message_loop_proxy.h:124: namespace internal { this can be moved to the .cc file now.
Beefed up the comments. I can't really fix the FROM_HERE thing because the FROM_HERE locaion objects use compile time constants for their vairous strings and data....so I can't just mangle the new information. Any suggestions? http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h File base/message_loop_proxy.h (right): http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h#n... base/message_loop_proxy.h:86: // while |task| may still be executing. On 2011/08/17 21:40:49, darin wrote: > it might help to have some examples here of how you use parameter currying to > capture results. it may also be helpful to show examples using WeakPtr. Added...but this comment is starting to get scarily large. http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h#n... base/message_loop_proxy.h:124: namespace internal { On 2011/08/17 21:40:49, darin wrote: > this can be moved to the .cc file now. Done.
On Wed, Aug 17, 2011 at 3:34 PM, <ajwong@chromium.org> wrote: > Beefed up the comments. > > I can't really fix the FROM_HERE thing because the FROM_HERE locaion > objects use > compile time constants for their vairous strings and data....so I can't > just > mangle the new information. > > Any suggestions? add a suffix/modifier field that is by default unset? > > > > http://codereview.chromium.**org/7210053/diff/29020/base/** > message_loop_proxy.h<http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h> > File base/message_loop_proxy.h (right): > > http://codereview.chromium.**org/7210053/diff/29020/base/** > message_loop_proxy.h#newcode86<http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h#newcode86> > base/message_loop_proxy.h:86: // while |task| may still be executing. > On 2011/08/17 21:40:49, darin wrote: > >> it might help to have some examples here of how you use parameter >> > currying to > >> capture results. it may also be helpful to show examples using >> > WeakPtr. > > Added...but this comment is starting to get scarily large. that's probably OK... people will appreciate the guidance long term, i really want to extract this function from BrowserThread and MessageLoopProxy and create a TaskRunner interface (per Will's suggestion). BrowserThread::IO()->PostTaskAndReply(...); The IO() method would return a TaskRunner pointer. Then, we would have one place to define and document the behavior of PostTask and friends. > > > http://codereview.chromium.**org/7210053/diff/29020/base/** > message_loop_proxy.h#**newcode124<http://codereview.chromium.org/7210053/diff/29020/base/message_loop_proxy.h#newcode124> > base/message_loop_proxy.h:124: namespace internal { > On 2011/08/17 21:40:49, darin wrote: > >> this can be moved to the .cc file now. >> > > Done. > > > http://codereview.chromium.**org/7210053/<http://codereview.chromium.org/7210... >
LGTM
Lgtm2! Thanks for all the hard work Albert. On Aug 17, 2011 3:42 PM, <darin@chromium.org> wrote: > LGTM > > http://codereview.chromium.org/7210053/
yay!! Will checkin after try bots to green. I filed a bug for the Location mangling. I hadn't thought about modifying Location to contain any more info. It's yet another 4 bytes to add to each FROM_HERE instance so I'm still a bit leery. But that's for another CL. On 2011/08/17 23:01:53, willchan wrote: > Lgtm2! Thanks for all the hard work Albert. > On Aug 17, 2011 3:42 PM, <mailto:darin@chromium.org> wrote: > > LGTM > > > > http://codereview.chromium.org/7210053/
Hey Nico, Mind looking at the valgrind suppression? -Albert
looks good I think…just make sure to watch the waterfall for a few h after landing http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... File tools/valgrind/memcheck/suppressions.txt (right): http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... tools/valgrind/memcheck/suppressions.txt:748: This test explicitly verifies PostTaskAndReply leaks the task if the originating MessageLoop has been deleted. No bug #? http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... tools/valgrind/memcheck/suppressions.txt:752: fun:_ZN4base12_GLOBAL__N_169MessageLoopProxyTest_PostTaskAndReply_DeadReplyLoopDoesNotDelete_Test8TestBodyEv GLOBAL might mangle differently on OS X, don't remember
http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... File tools/valgrind/memcheck/suppressions.txt (right): http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... tools/valgrind/memcheck/suppressions.txt:748: This test explicitly verifies PostTaskAndReply leaks the task if the originating MessageLoop has been deleted. On 2011/08/18 18:05:28, Nico wrote: > No bug #? I'm not sure it is a bug... http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... tools/valgrind/memcheck/suppressions.txt:752: fun:_ZN4base12_GLOBAL__N_169MessageLoopProxyTest_PostTaskAndReply_DeadReplyLoopDoesNotDelete_Test8TestBodyEv On 2011/08/18 18:05:28, Nico wrote: > GLOBAL might mangle differently on OS X, don't remember Okay I don't have a mac handy. Will watch the waterfall.
http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... File tools/valgrind/memcheck/suppressions.txt (right): http://codereview.chromium.org/7210053/diff/38003/tools/valgrind/memcheck/sup... tools/valgrind/memcheck/suppressions.txt:748: This test explicitly verifies PostTaskAndReply leaks the task if the originating MessageLoop has been deleted. On 2011/08/18 18:18:56, awong wrote: > On 2011/08/18 18:05:28, Nico wrote: > > No bug #? > I'm not sure it is a bug... I meant more "See http://crbug.com/86301" to save someone curious the `git blame` call in the future. This doesn't need a tracking bug, I agree. Just for reference.
Hey Nico, updated stuff. Let me know what you think.
valgrind change lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
