Move autowrap from ThreadManager constructor to Thread::Current.

webrtc/base/thread.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/base/thread.h"
 
 #if defined(WEBRTC_WIN)
 #include <comdef.h>
 #elif defined(WEBRTC_POSIX)
 #include <time.h>
 #endif
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/nullsocketserver.h"
 #include "webrtc/base/platform_thread.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/base/trace_event.h"
 
 namespace rtc {
 
 ThreadManager* ThreadManager::Instance() {
   RTC_DEFINE_STATIC_LOCAL(ThreadManager, thread_manager, ());

[tommi, 2017/04/21 09:38:58]

I hadn't seen this macro before. It doesn't say "LEAKY" in the name, although
the documentation for it says:

  "define a static local variable that gets leaked"

I think it would be an improvement to name it RTC_DEFINE_LEAKY_STATIC_LOCAL.

Then, to make things further more confusing (and arguably misleading), I see it
is used like this in one place:

RTC_DEFINE_STATIC_LOCAL(std::unique_ptr<RandomGenerator>, global_rng,
   (new SecureRandomGenerator()));

:-/

[nisse-webrtc, 2017/04/21 11:06:07]

I think I'd prefer to delete this macro, I don't think it improves clarity. But
that's for some other cl.

   return &thread_manager;
 }
 
+ThreadManager::~ThreadManager() {
+  // By above RTC_DEFINE_STATIC_LOCAL.
+  RTC_NOTREACHED() << "ThreadManager should never be destructed.";

[Taylor Brandstetter, 2017/04/21 09:54:28]

But it has a public constructor/destructor, so a native application could
construct it explicitly as opposed to using "Instance()". If we expect everyone
to go through "Instance", we should go a step further and make the constructor
private. Otherwise, we should remove the RTC_NOTREACHED(), and unwrap threads on
destruction.

[nisse-webrtc, 2017/04/21 11:06:07]

I'd prefer to not add the bookkeeping needed to unwrap threads. I could try
making constructor and destructor private. Do we have any other conventions on
how to do singletons (fancy name for global variable) like this?

BTW, according to
http://stackoverflow.com/questions/246564/what-is-the-lifetime-of-a-static-va...,
initialization of static locals like this are thread safe at least from C++11.
Which was news to me.

[Taylor Brandstetter, 2017/04/21 11:32:54]

Let's try making the constructor/destructor private and see if anyone notices. I
couldn't find any code that uses them.

I'm unaware of any singleton conventions we have, though just keeping
"Instance()" for now seems fine.

And yeah, static locals are pretty convenient. I don't think I've encountered
any problems yet with this approach to a singleton.

+}
+
 // static
 Thread* Thread::Current() {
+#ifdef NO_MAIN_THREAD_WRAPPING
   return ThreadManager::Instance()->CurrentThread();
+#else
+  return ThreadManager::Instance()->WrapCurrentThread();
+#endif
 }
 
 #if defined(WEBRTC_POSIX)
 #if !defined(WEBRTC_MAC)
 ThreadManager::ThreadManager() {
   pthread_key_create(&key_, nullptr);
-#ifndef NO_MAIN_THREAD_WRAPPING
-  WrapCurrentThread();
-#endif
-}
-
-ThreadManager::~ThreadManager() {
-  UnwrapCurrentThread();
-  pthread_key_delete(key_);
 }
 #endif
 
 Thread *ThreadManager::CurrentThread() {
   return static_cast<Thread *>(pthread_getspecific(key_));
 }
 
 void ThreadManager::SetCurrentThread(Thread *thread) {
   pthread_setspecific(key_, thread);
 }
 #endif
 
 #if defined(WEBRTC_WIN)
 ThreadManager::ThreadManager() {
   key_ = TlsAlloc();
-#ifndef NO_MAIN_THREAD_WRAPPING
-  WrapCurrentThread();
-#endif
-}
-
-ThreadManager::~ThreadManager() {
-  UnwrapCurrentThread();
-  TlsFree(key_);
 }
 
 Thread *ThreadManager::CurrentThread() {
   return static_cast<Thread *>(TlsGetValue(key_));
 }
 
 void ThreadManager::SetCurrentThread(Thread *thread) {
   TlsSetValue(key_, thread);
 }
 #endif
@@ skipping 445 matching lines @@
   if (SUCCEEDED(hr)) {
     Thread::Run();
     CoUninitialize();
   } else {
     LOG(LS_ERROR) << "CoInitialize failed, hr=" << hr;
   }
 }
 #endif
 
 }  // namespace rtc