Move Thread Name Mapping into ThreadFunc.

base/threading/thread.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/threading/thread.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/thread_id_name_manager.h"
@@ skipping 48 matching lines @@
       running_(false),
       startup_data_(NULL),
       thread_(0),
       message_loop_(NULL),
       thread_id_(kInvalidThreadId),
       name_(name) {
 }
 
 Thread::~Thread() {
   Stop();
-  ThreadIdNameManager::GetInstance()->RemoveName(thread_id_);
 }
 
 bool Thread::Start() {
   Options options;
 #if defined(OS_WIN)
   if (com_status_ == STA)
     options.message_loop_type = MessageLoop::TYPE_UI;
 #endif
   return StartWithOptions(options);
 }
@@ skipping 34 matching lines @@
 
   StopSoon();
 
   // Wait for the thread to exit.
   //
   // TODO(darin): Unfortunately, we need to keep message_loop_ around until
   // the thread exits.  Some consumers are abusing the API.  Make them stop.
   //
   PlatformThread::Join(thread_);
 
+  ThreadIdNameManager::GetInstance()->RemoveName(thread_id_);

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

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]

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]

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.

+
   // The thread should NULL message_loop_ on exit.
   DCHECK(!message_loop_);
 
   // The thread no longer needs to be joined.
   started_ = false;
 
   stopping_ = false;
 }
 
 void Thread::StopSoon() {
@@ skipping 72 matching lines @@
 
     // Assert that MessageLoop::Quit was called by ThreadQuitHelper.
     DCHECK(GetThreadWasQuitProperly());
 
     // We can't receive messages anymore.
     message_loop_ = NULL;
   }
 }
 
 }  // namespace base