Move Thread Name Mapping into ThreadFunc.

Move Thread Name Mapping into ThreadFunc.

This CL moves the setting and removal of the ThreadIdNameMapping into the ThreadFunc methods. This removes any potential race conditions around thread name removal as the thread will be live during both addition and removal.

BUG=238371
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202446

[dsinclair, 2013-05-07 16:55:44]

jar@ PTAL.

[dsinclair, 2013-05-08 18:56:15]

ping.

[jar (doing other things), 2013-05-14 16:49:10]

https://codereview.chromium.org/14634009/diff/1/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/14634009/diff/1/base/threading/thread.cc#newc...
base/threading/thread.cc:123:
ThreadIdNameManager::GetInstance()->RemoveName(thread_id_);
Given the problem you had... this solution is still racy :-(.

I suspect you need more state in the name manager, such as a back-pointer to
this instance... and then the delete name has to be careful to delete only the
name with the correct back pointer.

This does create a quandry, when someone is looking up the name :-/.

Perhaps what you need is a "most recently registered" (which is surely the most
current) as well as a reference count (so you only delete when the last
RemoveName is called.

...or am I missing something in the logic?

[dsinclair, 2013-05-14 17:00:17]

https://codereview.chromium.org/14634009/diff/1/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/14634009/diff/1/base/threading/thread.cc#newc...
base/threading/thread.cc:123:
ThreadIdNameManager::GetInstance()->RemoveName(thread_id_);
On 2013/05/14 16:49:10, jar wrote:
> Given the problem you had... this solution is still racy :-(.
> 
> I suspect you need more state in the name manager, such as a back-pointer to
> this instance... and then the delete name has to be careful to delete only the
> name with the correct back pointer.
> 
> This does create a quandry, when someone is looking up the name :-/.
> 
> Perhaps what you need is a "most recently registered" (which is surely the
most
> current) as well as a reference count (so you only delete when the last
> RemoveName is called.
> 
> ...or am I missing something in the logic?


You're right, there is still a race here.

Looking at the code, the simplest solution seems like it would be to move this
line up above the StopSoon() call. That way, the thread is shutting down, but
hasn't executed the code to quit the message loop so the thread_id should still
be allocated to this thread. (Unless I'm failing to understand the code flow
here.)

Does that make sense? This way there should only be one instance of any
thread_id getting a name, so we don't have to change the manager.

[jar (doing other things), 2013-05-14 19:55:06]

https://codereview.chromium.org/14634009/diff/1/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/14634009/diff/1/base/threading/thread.cc#newc...
base/threading/thread.cc:123:
ThreadIdNameManager::GetInstance()->RemoveName(thread_id_);
On 2013/05/14 17:00:17, dsinclair wrote:
> On 2013/05/14 16:49:10, jar wrote:
> > Given the problem you had... this solution is still racy :-(.
> > 
> > I suspect you need more state in the name manager, such as a back-pointer to
> > this instance... and then the delete name has to be careful to delete only
the
> > name with the correct back pointer.
> > 
> > This does create a quandry, when someone is looking up the name :-/.
> > 
> > Perhaps what you need is a "most recently registered" (which is surely the
> most
> > current) as well as a reference count (so you only delete when the last
> > RemoveName is called.
> > 
> > ...or am I missing something in the logic?
> 
> 
> You're right, there is still a race here.
> 
> Looking at the code, the simplest solution seems like it would be to move this
> line up above the StopSoon() call. That way, the thread is shutting down, but
> hasn't executed the code to quit the message loop so the thread_id should
still
> be allocated to this thread. (Unless I'm failing to understand the code flow
> here.)
> 
> Does that make sense? This way there should only be one instance of any
> thread_id getting a name, so we don't have to change the manager.

I don't fully grok the use case... but I suspect you don't want to unregister
while the thread is active... and may be running task after task after task :-(.
 That is sadly the case before and after StopSoon().

You could do the unregister in the actual thread shutdown code, such as when the
message loop unwinds.  That would probably get rid of the race, and be safe. You
can't start a new thread until you "know" that the old thread using the same
name has terminated... and the decision to terminate is made in the message loop
(or message pump) code.

[dsinclair, 2013-05-16 20:07:51]

On 2013/05/14 19:55:06, jar wrote:
> You could do the unregister in the actual thread shutdown code, such as when
the
> message loop unwinds.  That would probably get rid of the race, and be safe.
You
> can't start a new thread until you "know" that the old thread using the same
> name has terminated... and the decision to terminate is made in the message
loop
> (or message pump) code.

PTAL.

I went back to your original suggestion and added some extra tracking into the
name manager. Now, I associate the thread name to the PlatformThreadHandle. I
then associate the thread_id to the handle. When I want to look up the name for
a thread_id I first look up the handle then use that to find the name.

When we remove the thread name I remove the name associated with the handle and,
if the id still points to the given handle, I'll remove the id otherwise I leave
it as the thread_id has been reused.

I modified the ThreadFunc methods in the PlatformThread code as that was the
only place I could find where I could get access to the thread_id and the handle
at the same time. I can't use ThreadMain as that may not be called if a delegate
is provided.

[dsinclair, 2013-05-22 14:11:59]

ping.

[jar (doing other things), 2013-05-22 19:37:55]

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
base/threading/platform_thread_win.cc:63: return NULL;
Wouldn't this be a great place to unregister, just in front of line 63?  ...
while the thread is still active... but about to terminate.... so it would be a
bad thing for folks to refer to us anyway IMO.

[Same thing in platform_thread_posix.cc]

https://codereview.chromium.org/14634009/diff/28001/base/threading/simple_thr...
File base/threading/simple_thread.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/simple_thr...
base/threading/simple_thread.cc:43:
ThreadIdNameManager::GetInstance()->RemoveName(&thread_, tid_);
This still scares me a bit if I'm worried about races.  This will run after an
arbitrary amount of time has passed since the thread has gone away.... which
means some other thread may have started, and gotten the previous one's id.

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:20: typedef std::map<PlatformThreadId,
PlatformThreadHandle*>::iterator
nit: My preference is to typedef the actual map type, probably as a private
typedef in the header file, and then use that to both declare the slots, and use
it to spec an iterator (and not bother to have a typedef for the iterator.

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:72: main_process_id_ = id;
Assuming there are no misunderstanding (and races), we should only set this
value exactly once, correct?  Can we CHECK() on this?

Should we also add this id to the thread_id_to_handle_ map?  ...and to the
handle_to_interned_name_ map?

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:82: return main_process_name_->c_str();
If we had put it in the map.... we could just use the general code.  Was there a
reason not to?

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:44: // lock_ protects both the
thread_id_to_name_ and name_to_interned_name_ maps.
You probably want to update this comment, and your maps should also be protected
by this lock.

[dsinclair, 2013-05-22 20:22:31]

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
base/threading/platform_thread_win.cc:63: return NULL;
On 2013/05/22 19:37:56, jar wrote:
> Wouldn't this be a great place to unregister, just in front of line 63?  ...
> while the thread is still active... but about to terminate.... so it would be
a
> bad thing for folks to refer to us anyway IMO.
> 
> [Same thing in platform_thread_posix.cc]

The benefit I see from having it in the Join method of PlatformThread is that we
know all posted tasks are done. My concern was that, things like
MessageLoop::PostTask, use the thread name when they do TRACE_EVENT calls.

https://codereview.chromium.org/14634009/diff/28001/base/threading/simple_thr...
File base/threading/simple_thread.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/simple_thr...
base/threading/simple_thread.cc:43:
ThreadIdNameManager::GetInstance()->RemoveName(&thread_, tid_);
On 2013/05/22 19:37:56, jar wrote:
> This still scares me a bit if I'm worried about races.  This will run after an
> arbitrary amount of time has passed since the thread has gone away.... which
> means some other thread may have started, and gotten the previous one's id.

The ThreadIdNameManager code takes care of this. We remove the thread -> name
pointer which will only be referenced by us. We then only remove the id ->
thread map entry if thread is equal to the provided thread. 

This way, if another thread has come along, it will change id->thread and we'll
leave it, but we'll still clean up our thread->name mapping.

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:20: typedef std::map<PlatformThreadId,
PlatformThreadHandle*>::iterator
On 2013/05/22 19:37:56, jar wrote:
> nit: My preference is to typedef the actual map type, probably as a private
> typedef in the header file, and then use that to both declare the slots, and
use
> it to spec an iterator (and not bother to have a typedef for the iterator.

Done.

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:72: main_process_id_ = id;
On 2013/05/22 19:37:56, jar wrote:
> Assuming there are no misunderstanding (and races), we should only set this
> value exactly once, correct?  Can we CHECK() on this?
> 
> Should we also add this id to the thread_id_to_handle_ map?  ...and to the
> handle_to_interned_name_ map?

It could be set multiple times. I think in practice we only call
PlatformThread::SetName once, but there is nothing that stops us from calling it
over and over again.

I can't put the main process information into the maps because I don't have a
PlatformThreadHandle for the main process. The two maps both use the Handle
(thread_id -> handle, handle -> name).

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:82: return main_process_name_->c_str();
On 2013/05/22 19:37:56, jar wrote:
> If we had put it in the map.... we could just use the general code.  Was there
a
> reason not to?

I can't put it into the map without a PlatformThreadHandle, as far as I can
tell, there is no handle for the main thread?

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:44: // lock_ protects both the
thread_id_to_name_ and name_to_interned_name_ maps.
On 2013/05/22 19:37:56, jar wrote:
> You probably want to update this comment, and your maps should also be
protected
> by this lock.

Done.

[jar (doing other things), 2013-05-22 21:36:47]

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
base/threading/platform_thread_win.cc:63: return NULL;
On 2013/05/22 20:22:31, dsinclair wrote:
> On 2013/05/22 19:37:56, jar wrote:
> > Wouldn't this be a great place to unregister, just in front of line 63?  ...
> > while the thread is still active... but about to terminate.... so it would
be
> a
> > bad thing for folks to refer to us anyway IMO.
> > 
> > [Same thing in platform_thread_posix.cc]
> 
> The benefit I see from having it in the Join method of PlatformThread is that
we
> know all posted tasks are done. My concern was that, things like
> MessageLoop::PostTask, use the thread name when they do TRACE_EVENT calls.

I think that by the time you hit line 63, any message loop on this thread should
be gone... so the question is: Who can/should be referencing the thread, and
asking questions about it?  Surely we won't be askin' any more questions
(tracing any more calls), as we're exiting here.

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:99: if (*handle ==
base::kNullThreadHandle)
I see you deleted this (presumably because it can never happen).  What are the
cases where line 101 fires?

[dsinclair, 2013-05-23 18:21:53]

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/platform_t...
base/threading/platform_thread_win.cc:63: return NULL;
On 2013/05/22 21:36:48, jar wrote:
> On 2013/05/22 20:22:31, dsinclair wrote:
> > On 2013/05/22 19:37:56, jar wrote:
> > > Wouldn't this be a great place to unregister, just in front of line 63? 
...
> > > while the thread is still active... but about to terminate.... so it would
> be
> > a
> > > bad thing for folks to refer to us anyway IMO.
> > > 
> > > [Same thing in platform_thread_posix.cc]
> > 
> > The benefit I see from having it in the Join method of PlatformThread is
that
> we
> > know all posted tasks are done. My concern was that, things like
> > MessageLoop::PostTask, use the thread name when they do TRACE_EVENT calls.
> 
> I think that by the time you hit line 63, any message loop on this thread
should
> be gone... so the question is: Who can/should be referencing the thread, and
> asking questions about it?  Surely we won't be askin' any more questions
> (tracing any more calls), as we're exiting here.
> 

Done.

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/14634009/diff/28001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:99: if (*handle ==
base::kNullThreadHandle)
On 2013/05/22 21:36:48, jar wrote:
> I see you deleted this (presumably because it can never happen).  What are the
> cases where line 101 fires?

I dropped it originally because base::kNullThreadHandle doesn't exist anymore.

I've updated the RegisterThread code so that we always provide kDefaultName into
the handle to name map. This removes the need to special case not finding it in
GetName and keeps the remove code simpler.

[jar (doing other things), 2013-05-25 00:25:09]

Just nits... and then LGTM.

https://codereview.chromium.org/14634009/diff/83001/base/threading/simple_thr...
File base/threading/simple_thread.cc (right):

https://codereview.chromium.org/14634009/diff/83001/base/threading/simple_thr...
base/threading/simple_thread.cc:10: #include
"base/threading/thread_id_name_manager.h"
nit: Do you still need this include?

https://codereview.chromium.org/14634009/diff/83001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/14634009/diff/83001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:58: // Treat the main process specially
as it doesn't have a PlatformThreadHandle
nit: period at end of sentence... and you might have to wrap the line the line
at 80 columns if you can't be more terns.

[commit-bot: I haz the power, 2013-05-27 13:53:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/14634009/94001

[commit-bot: I haz the power, 2013-05-27 16:37:40]

Change committed as 202446

[dsinclair, 2013-05-27 18:50:59]

https://codereview.chromium.org/14634009/diff/83001/base/threading/simple_thr...
File base/threading/simple_thread.cc (right):

https://codereview.chromium.org/14634009/diff/83001/base/threading/simple_thr...
base/threading/simple_thread.cc:10: #include
"base/threading/thread_id_name_manager.h"
On 2013/05/25 00:25:09, jar wrote:
> nit: Do you still need this include?

Nope. Done.

https://codereview.chromium.org/14634009/diff/83001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/14634009/diff/83001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:58: // Treat the main process specially
as it doesn't have a PlatformThreadHandle
On 2013/05/25 00:25:09, jar wrote:
> nit: period at end of sentence... and you might have to wrap the line the line
> at 80 columns if you can't be more terns.

Done.