folly: ThreadLocalDetail: make PthreadKeyUnregister constexpr-constructible, avoid...

Summary:
Since this is used in so many places during the program's static-initialization (at startup), this class itself could be (has been demonstrated to be) a point of SIOF problems itself.

Made this class constexpr-constructible, so it doesn't need to be part of static initialization, making it SIOF-proof.

Reviewed By: luciang

Differential Revision: D2709231

fb-gh-sync-id: f248c9f2848c09045e000cfdc03636d847e522c9

diff --git a/folly/detail/ThreadLocalDetail.cpp b/folly/detail/ThreadLocalDetail.cpp
index 581563650cb01eebef6f007c6b5b1c0c1468131f..0f6e84a6538d4256ace082bed95fc76fe4cfddde 100644 (file)
--- a/folly/detail/ThreadLocalDetail.cpp
+++ b/folly/detail/ThreadLocalDetail.cpp
@@ -17,7 +17,6 @@
 
 namespace folly { namespace threadlocal_detail {
 
-PthreadKeyUnregister
-MAX_STATIC_CONSTRUCTOR_PRIORITY PthreadKeyUnregister::instance_;
+PthreadKeyUnregister PthreadKeyUnregister::instance_;
 
 }}

diff --git a/folly/detail/ThreadLocalDetail.h b/folly/detail/ThreadLocalDetail.h
index 999d169d7bdcc8752d059d7242a499e06ff87f15..69d2cabd0d3c1baf73802405fcf5bc645d107e58 100644 (file)
--- a/folly/detail/ThreadLocalDetail.h
+++ b/folly/detail/ThreadLocalDetail.h
@@ -163,20 +163,29 @@ struct ThreadEntry {
 
 constexpr uint32_t kEntryIDInvalid = std::numeric_limits<uint32_t>::max();
 
+struct PthreadKeyUnregisterTester;
+
 /**
  * We want to disable onThreadExit call at the end of shutdown, we don't care
  * about leaking memory at that point.
  *
  * Otherwise if ThreadLocal is used in a shared library, onThreadExit may be
  * called after dlclose().
+ *
+ * This class has one single static instance; however since it's so widely used,
+ * directly or indirectly, by so many classes, we need to take care to avoid
+ * problems stemming from the Static Initialization/Destruction Order Fiascos.
+ * Therefore this class needs to be constexpr-constructible, so as to avoid
+ * the need for this to participate in init/destruction order.
  */
 class PthreadKeyUnregister {
  public:
+  static constexpr size_t kMaxKeys = 1UL << 16;
+
   ~PthreadKeyUnregister() {
     std::lock_guard<std::mutex> lg(mutex_);
-
-    for (const auto& key: keys_) {
-      pthread_key_delete(key);
+    while (size_--) {
+      pthread_key_delete(keys_[size_]);
     }
   }
 
@@ -185,21 +194,27 @@ class PthreadKeyUnregister {
   }
 
  private:
-  PthreadKeyUnregister() {}
+  /**
+   * Only one global instance should exist, hence this is private.
+   * See also the important note at the top of this class about `constexpr`
+   * usage.
+   */
+  constexpr PthreadKeyUnregister() : mutex_(), size_(0), keys_() { }
+  friend class folly::threadlocal_detail::PthreadKeyUnregisterTester;
 
   void registerKeyImpl(pthread_key_t key) {
     std::lock_guard<std::mutex> lg(mutex_);
-
-    keys_.push_back(key);
+    CHECK_LT(size_, kMaxKeys);
+    keys_[size_++] = key;
   }
 
   std::mutex mutex_;
-  std::vector<pthread_key_t> keys_;
+  size_t size_;
+  pthread_key_t keys_[kMaxKeys];
 
   static PthreadKeyUnregister instance_;
 };
 
-
 // Held in a singleton to track our global instances.
 // We have one of these per "Tag", by default one for the whole system
 // (Tag=void).

diff --git a/folly/test/ThreadLocalTest.cpp b/folly/test/ThreadLocalTest.cpp
index 1f089fa6144dcd97190c8c88532bba5ffebb5fd0..0d4909791dd6ef7ad5fd6361dda71f9705c4196a 100644 (file)
--- a/folly/test/ThreadLocalTest.cpp
+++ b/folly/test/ThreadLocalTest.cpp
@@ -583,6 +583,19 @@ TEST(ThreadLocal, SharedLibrary) {
   t2.join();
 }
 
+namespace folly { namespace threadlocal_detail {
+struct PthreadKeyUnregisterTester {
+  PthreadKeyUnregister p;
+  constexpr PthreadKeyUnregisterTester() = default;
+};
+}}
+
+TEST(ThreadLocal, UnregisterClassHasConstExprCtor) {
+  folly::threadlocal_detail::PthreadKeyUnregisterTester x;
+  // yep!
+  SUCCEED();
+}
+
 // clang is unable to compile this code unless in c++14 mode.
 #if __cplusplus >= 201402L
 namespace {