--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
If you find yourself caching a MessageLoop* in your class simply so you can post tasks to a specific thread or so that you can check it against the current thread like so:DCHECK_EQ(MessageLoop::current(), message_loop_);then, please use a scoped_refptr<MessageLoopProxy> instead. MessageLoopProxy can outlive the target thread and is a safe way to hold on to message loop pointers and leads to far less brittle code. If you care whether your PostTask actually was posted to the target thread or vanished into oblivion, then you can check the return value.
Once again, please do not cache MessageLoop pointers unless you absolutely absolutely have to (actually I can't really think of a good reason to hold on to a MessageLoop*).
--
OTOH, even if the message loop already contains a QuitTask, it will
still run *all* tasks until it would block otherwise
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/base/message_loop.cc&l=248).
On Thu, Apr 7, 2011 at 11:24, Jonathan Dixon <jo...@chromium.org> wrote:OTOH, even if the message loop already contains a QuitTask, it will
>
>
> On 7 April 2011 06:03, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
>>
>> If you find yourself caching a MessageLoop* in your class simply so you
>> can post tasks to a specific thread or so that you can check it against the
>> current thread like so:
>> DCHECK_EQ(MessageLoop::current(), message_loop_);
>> then, please use a scoped_refptr<MessageLoopProxy> instead.
>> MessageLoopProxy can outlive the target thread and is a safe way to hold on
>> to message loop pointers and leads to far less brittle code. If you care
>> whether your PostTask actually was posted to the target thread or vanished
>> into oblivion, then you can check the return value.
>
> ...but of you do: Note that even if the task is posted, there's no guarantee
> that it will run, since the target thread may already have a Quit message in
> its queue.
> http://src.chromium.org/viewvc/chrome/trunk/src/base/message_loop_proxy.h?revision=79524&view=markup
still run *all* tasks until it would block otherwise
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/base/message_loop.cc&l=248).
// These methods are the same as in message_loop.h, but are guaranteed to // either post the Task to the MessageLoop (if it's still alive), or to // delete the Task otherwise. // They return true iff the thread existed and the task was posted. Note that // even if the task is posted, there's no guarantee that it will run, since // the target thread may already have a Quit message in its queue. virtual bool PostTask(const tracked_objects::Location& from_here, Task* task) = 0;
On Thu, Apr 7, 2011 at 11:24, Jonathan Dixon <jo...@chromium.org> wrote:OTOH, even if the message loop already contains a QuitTask, it will
>
>
> On 7 April 2011 06:03, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
>>
>> If you find yourself caching a MessageLoop* in your class simply so you
>> can post tasks to a specific thread or so that you can check it against the
>> current thread like so:
>> DCHECK_EQ(MessageLoop::current(), message_loop_);
>> then, please use a scoped_refptr<MessageLoopProxy> instead.
>> MessageLoopProxy can outlive the target thread and is a safe way to hold on
>> to message loop pointers and leads to far less brittle code. If you care
>> whether your PostTask actually was posted to the target thread or vanished
>> into oblivion, then you can check the return value.
>
> ...but of you do: Note that even if the task is posted, there's no guarantee
> that it will run, since the target thread may already have a Quit message in
> its queue.
> http://src.chromium.org/viewvc/chrome/trunk/src/base/message_loop_proxy.h?revision=79524&view=markup
still run *all* tasks until it would block otherwise
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/base/message_loop.cc&l=248).
{ // The message loop for this thread. MessageLoop message_loop(startup_data_->options.message_loop_type); // Complete the initialization of our Thread object. thread_id_ = PlatformThread::CurrentId(); PlatformThread::SetName(name_.c_str()); ANNOTATE_THREAD_NAME(name_.c_str()); // Tell the name to race detector. message_loop.set_thread_name(name_); message_loop_ = &message_loop; message_loop_proxy_ = MessageLoopProxy::CreateForCurrentThread(); // Let the thread do extra initialization. // Let's do this before signaling we are started. Init(); startup_data_->event.Signal(); // startup_data_ can't be touched anymore since the starting thread is now // unlocked. Run(message_loop_); // Let the thread do extra cleanup. CleanUp(); // Assert that MessageLoop::Quit was called by ThreadQuitTask. DCHECK(GetThreadWasQuitProperly()); // We can't receive messages anymore. message_loop_ = NULL; message_loop_proxy_ = NULL; }
Bottom line, just because you post a task doesn't mean it'll get run. MessageLoopProxy::PostTask() returning false just lets you know for sure that it's not getting run :P
Yeah, I wondered about that as well. Maybe we should just remove the
"since the target thread may already have a Quit message in its queue"
part but still keep the comment that the task may not actually run (or
add Will's explanation instead)? That way, we don't create false
expectations about the reasons for not running the task.
There is some useful info here btw.:
(and it also mentions QuitTask .. maybe it needs editing too?)
--Craig
Reiterating what Will said: There is no difference in the guarantee of the task executing between using MessageLoop and MessageLoopProxy. What MessageLoopProxy guarantees is that it will not crash if the MessageLoop is gone.
Darin, I agree with the "it is better to crash right away rather than limp along" philosophy. However, to do that I would rather use a CHECK for the return value of MessageLoopProxy::PostTask rather than rely on the crash on accessing an invalid pointer. In fact, the main reason that prompted my email was a debugging experience I had yesterday. A unit-test was crashing strangely on one of the trybots. The crash was nowhere near any of the code I had touched, nor did it have a meaningful call stack. The problem happened because of some code in HostResolver acccessing a MessageLoop* after it had gone. It did not crash right away but caused a memory corruption that caused a crash much later in an unrelated place. The only I found this was by enabling App Verifier on the unit-test executable and running it under WinDbg. When I did this, it crashed right away at the right place. (As an aside, App Verifier is a wonderful tool for such cases on Windows, it is a pity we don't use it more often).
Most of the points have already been raised, I'd just like to add that checking the result of PostTask/DeleteSoon/ReleaseSoon etc isn't a pattern that I think will help the code. IMO it's as useful as DCHECKing/CHECKING pointers in the top of a function before using them.
On Thu, Apr 7, 2011 at 10:44 AM, John Abd-El-Malek <j...@chromium.org> wrote:Most of the points have already been raised, I'd just like to add that checking the result of PostTask/DeleteSoon/ReleaseSoon etc isn't a pattern that I think will help the code. IMO it's as useful as DCHECKing/CHECKING pointers in the top of a function before using them.Agreed. My point above was simply comparing 2 scenarios: Crash because of accessing a junk MessageLoop* and Crash because of CHECKing the return value of MessageLoopProxy::PostTask. What I was saying is that, if you want to crash in this scenario, the second method is a more predictable way to crash. Whether you want to crash at all in these scenarios is a different issue.
Wow, seems like quite a debate started here. Lemme chime in with my point of view.
Executive summary:
MessageLoopProxy/BrowserThread take shutdown crash / heap corruption bugs and turn them into leaks (which are bugs too!). They're bandaids. Use them, but always think about how your code is supposed to behave on shutdown.
<rant>
First, all this shutdown crap is a fool's errand. The right way to handle this is to just exit the process after we've synced any important data to disk. See http://code.google.com/p/chromium/issues/detail?id=43782 for details. The problem is, no one's assigned to work on it. It's a difficult PITA (our shutdown code is absolute madness, may whatever spiritual deity pity the lunatic who tries to understand it) and fixing it isn't the most glorious of tasks. But it'd fix a whole lot of issues and speed up shutdown.
Back to reality. Posting a task where you don't know that it'll run successfully is a bug. You can't tell from MessageLoop::PostTask() or MessageLoopProxy::PostTask() if it'll run successfully as we've already noted. MessageLoopProxy/BrowserThread are designed to take these bugs, that previously caused crashes / heap corruption, and turned them into leaks. In almost all cases, this is a better user experience. It's still a leak. Something that may or may not have been important to run now is not getting run. Valgrind complains. Have you seen our valgrind suppressions file? Ugh.
MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.
Lest I seem like I'm coming off too harsh, let me reassert that crashing is generally worse. At least we're still flushing our data to disk and not cluttering the crash reports and causing flaky tests. Only valgrind complains and the minor user experience difference is _usually_ not noticed since you're shutting down anyways, so does it really matter? In code reviews, I will almost always tell someone to use MessageLoopProxy/BrowserThread. But I will also ask them what they think is going to happen on shutdown. Do they even know the order of thread shutdown in Chrome? Generally, they haven't thought about it. This is a problem.
</rant>
If you find yourself caching a MessageLoop* in your class simply so you can post tasks to a specific thread or so that you can check it against the current thread like so:DCHECK_EQ(MessageLoop::current(), message_loop_);then, please use a scoped_refptr<MessageLoopProxy> instead. MessageLoopProxy can outlive the target thread and is a safe way to hold on to message loop pointers and leads to far less brittle code. If you care whether your PostTask actually was posted to the target thread or vanished into oblivion, then you can check the return value.
Once again, please do not cache MessageLoop pointers unless you absolutely absolutely have to (actually I can't really think of a good reason to hold on to a MessageLoop*).
Wow, seems like quite a debate started here. Lemme chime in with my point of view.
Executive summary:
MessageLoopProxy/BrowserThread take shutdown crash / heap corruption bugs and turn them into leaks (which are bugs too!). They're bandaids. Use them, but always think about how your code is supposed to behave on shutdown.
<rant>
First, all this shutdown crap is a fool's errand. The right way to handle this is to just exit the process after we've synced any important data to disk. See http://code.google.com/p/chromium/issues/detail?id=43782 for details. The problem is, no one's assigned to work on it. It's a difficult PITA (our shutdown code is absolute madness, may whatever spiritual deity pity the lunatic who tries to understand it) and fixing it isn't the most glorious of tasks. But it'd fix a whole lot of issues and speed up shutdown.
Back to reality. Posting a task where you don't know that it'll run successfully is a bug. You can't tell from MessageLoop::PostTask() or MessageLoopProxy::PostTask() if it'll run successfully as we've already noted. MessageLoopProxy/BrowserThread are designed to take these bugs, that previously caused crashes / heap corruption, and turned them into leaks. In almost all cases, this is a better user experience. It's still a leak. Something that may or may not have been important to run now is not getting run. Valgrind complains. Have you seen our valgrind suppressions file? Ugh.
MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.
Lest I seem like I'm coming off too harsh, let me reassert that crashing is generally worse. At least we're still flushing our data to disk and not cluttering the crash reports and causing flaky tests. Only valgrind complains and the minor user experience difference is _usually_ not noticed since you're shutting down anyways, so does it really matter? In code reviews, I will almost always tell someone to use MessageLoopProxy/BrowserThread. But I will also ask them what they think is going to happen on shutdown. Do they even know the order of thread shutdown in Chrome? Generally, they haven't thought about it. This is a problem.
Developer/testing builds could shut down normally most of the time
(based on a flag) and we could keep e.g. a 1% per-startup randomized
experiment on our whole user base (or dev + beta) going so that we
could continue to collect problems with normal shutdown from the wild.
Cheers,
Jói
MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.
Lest I seem like I'm coming off too harsh, let me reassert that crashing is generally worse. At least we're still flushing our data to disk and not cluttering the crash reports and causing flaky tests.
On Thu, Apr 7, 2011 at 22:10, William Chan (陈智昌) <will...@chromium.org> wrote:MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.
Does "guaranteed" mean it'll always leak? Also, doesn't the UI thread outlive all the other threads?
I was under the impression that worker threads (via base::WorkerPool)
could outlive the UI thread, but that yes, all well-known threads have
exited their message loop before the UI thread ends.
Cheers,
Jói
On Mon, Apr 11, 2011 at 1:33 PM, William Chan (陈智昌)
> Randy asked me about this too. Yes! The UI thread outlives all other> threads. [...]
I was under the impression that worker threads (via base::WorkerPool)
could outlive the UI thread, but that yes, all well-known threads have
exited their message loop before the UI thread ends.
Darin wrote:
> [...] Fast
> shutdown is in conflict though with leak checking, and I'm not sure how toDeveloper/testing builds could shut down normally most of the time
> deal effectively with that.
(based on a flag) and we could keep e.g. a 1% per-startup randomized
experiment on our whole user base (or dev + beta) going so that we
could continue to collect problems with normal shutdown from the wild.