Revert D8397670 D8797823

Summary:
Delayed hazptr destruction is causing destruction ordering issues.

P59812860$445

buck test //titan/cachius/test:CachiusServiceHandlerTest

Reviewed By: phil-lopreiato

Differential Revision: D8825716

fbshipit-source-id: 808341489fd80b8edb63eb7c12377a8ffadfd1eb

folly/io/async/Request.cpp

@@ -21,51 +21,18 @@
 
 #include <folly/MapUtil.h>
 #include <folly/SingletonThreadLocal.h>
-#include <folly/synchronization/CallOnce.h>
-#include <folly/synchronization/Hazptr.h>
 
 namespace folly {
 
-namespace {
-
-/*
- * Some requests may not be deleted until long after main when the
- * default hazptr_domain destructs.  This may cause destruction
- * ordering issues.  Instead, cleanup as soon as possible after main
- * using atexit.
- */
-folly::once_flag atexitflag;
-void cleanup(void) {
-  atexit([]() { hazptr_cleanup(); });
-}
-
-} // namespace
-
-struct RequestContext::State : hazptr_obj_base<State> {
-  std::map<std::string, std::shared_ptr<RequestData>> requestData_;
-  std::set<RequestData*> callbackData_;
-};
-
-RequestContext::RequestContext() : state_(new State) {}
-
-RequestContext::~RequestContext() {
-  auto p = state_.load(std::memory_order_relaxed);
-  delete p;
-}
-
 bool RequestContext::doSetContextData(
     const std::string& val,
     std::unique_ptr<RequestData>& data,
     bool strict) {
-  State* p{nullptr};
-
-  {
-    std::lock_guard<std::mutex> g(m_);
-    p = state_.load(std::memory_order_acquire);
+  auto ulock = state_.ulock();
 
   bool conflict = false;
-    auto it = p->requestData_.find(val);
-    if (it != p->requestData_.end()) {
+  auto it = ulock->requestData_.find(val);
+  if (it != ulock->requestData_.end()) {
     if (strict) {
       return false;
     } else {
@@ -75,31 +42,23 @@ bool RequestContext::doSetContextData(
     }
   }
 
-    auto newstate = new State(*p);
-    it = newstate->requestData_.find(val);
-
+  auto wlock = ulock.moveFromUpgradeToWrite();
   if (conflict) {
     if (it->second) {
       if (it->second->hasCallback()) {
         it->second->onUnset();
-          newstate->callbackData_.erase(it->second.get());
+        wlock->callbackData_.erase(it->second.get());
       }
-        it->second.reset();
+      it->second.reset(nullptr);
     }
+    return true;
   }
 
   if (data && data->hasCallback()) {
-      newstate->callbackData_.insert(data.get());
+    wlock->callbackData_.insert(data.get());
     data->onSet();
   }
-    newstate->requestData_[val] = std::move(data);
-    state_.store(newstate, std::memory_order_release);
-  }
-
-  if (p) {
-    p->retire();
-    folly::call_once(atexitflag, cleanup);
-  }
+  wlock->requestData_[val] = std::move(data);
 
   return true;
 }
@@ -117,49 +76,38 @@ bool RequestContext::setContextDataIfAbsent(
 }
 
 bool RequestContext::hasContextData(const std::string& val) const {
-  hazptr_local<1> hptr;
-  auto p = hptr[0].get_protected(state_);
-  return p->requestData_.count(val) != 0;
+  return state_.rlock()->requestData_.count(val);
 }
 
 RequestData* RequestContext::getContextData(const std::string& val) {
-  const std::shared_ptr<RequestData> dflt{nullptr};
-  hazptr_local<1> hptr;
-  auto p = hptr[0].get_protected(state_);
-  p->requestData_.count(val);
-  return get_ref_default(p->requestData_, val, dflt).get();
+  const std::unique_ptr<RequestData> dflt{nullptr};
+  return get_ref_default(state_.rlock()->requestData_, val, dflt).get();
 }
 
 const RequestData* RequestContext::getContextData(
     const std::string& val) const {
-  const std::shared_ptr<RequestData> dflt{nullptr};
-  hazptr_local<1> hptr;
-  auto p = hptr[0].get_protected(state_);
-  p->requestData_.count(val);
-  return get_ref_default(p->requestData_, val, dflt).get();
+  const std::unique_ptr<RequestData> dflt{nullptr};
+  return get_ref_default(state_.rlock()->requestData_, val, dflt).get();
 }
 
 void RequestContext::onSet() {
-  hazptr_holder<> hptr;
-  auto p = hptr.get_protected(state_);
-  for (const auto& data : p->callbackData_) {
+  auto rlock = state_.rlock();
+  for (const auto& data : rlock->callbackData_) {
     data->onSet();
   }
 }
 
 void RequestContext::onUnset() {
-  hazptr_holder<> hptr;
-  auto p = hptr.get_protected(state_);
-  for (const auto& data : p->callbackData_) {
+  auto rlock = state_.rlock();
+  for (const auto& data : rlock->callbackData_) {
     data->onUnset();
   }
 }
 
 std::shared_ptr<RequestContext> RequestContext::createChild() {
-  hazptr_local<1> hptr;
-  auto p = hptr[0].get_protected(state_);
   auto child = std::make_shared<RequestContext>();
-  for (const auto& entry : p->requestData_) {
+  auto rlock = state_.rlock();
+  for (const auto& entry : rlock->requestData_) {
     auto& key = entry.first;
     auto childData = entry.second->createChild();
     if (childData) {
@@ -170,38 +118,24 @@ std::shared_ptr<RequestContext> RequestContext::createChild() {
 }
 
 void RequestContext::clearContextData(const std::string& val) {
-  std::shared_ptr<RequestData> requestData;
-  // requestData is deleted while lock is not held, in case
-  // destructor contains calls to RequestContext
-  State* p{nullptr};
-
+  std::unique_ptr<RequestData> requestData;
+  // Delete the RequestData after giving up the wlock just in case one of the
+  // RequestData destructors will try to grab the lock again.
   {
-    std::lock_guard<std::mutex> g(m_);
-    p = state_.load(std::memory_order_acquire);
-
-    auto it = p->requestData_.find(val);
-    if (it == p->requestData_.end()) {
+    auto ulock = state_.ulock();
+    auto it = ulock->requestData_.find(val);
+    if (it == ulock->requestData_.end()) {
       return;
     }
 
-    auto newstate = new State(*p);
-
-    it = newstate->requestData_.find(val);
-    CHECK(it != newstate->requestData_.end());
-
+    auto wlock = ulock.moveFromUpgradeToWrite();
     if (it->second && it->second->hasCallback()) {
       it->second->onUnset();
-      newstate->callbackData_.erase(it->second.get());
-    }
-
-    requestData = it->second;
-    newstate->requestData_.erase(it);
-    state_.store(newstate, std::memory_order_release);
+      wlock->callbackData_.erase(it->second.get());
     }
 
-  if (p) {
-    p->retire();
-    folly::call_once(atexitflag, cleanup);
+    requestData = std::move(it->second);
+    wlock->requestData_.erase(it);
   }
 }
 

folly/io/async/Request.h

@@ -16,12 +16,12 @@
 
 #pragma once
 
-#include <atomic>
 #include <map>
 #include <memory>
-#include <mutex>
 #include <set>
 
+#include <folly/Synchronized.h>
+
 namespace folly {
 
 // Some request context that follows an async request through a process
@@ -58,8 +58,6 @@ class RequestData {
 // copied between threads.
 class RequestContext {
  public:
-  RequestContext();
-  ~RequestContext();
   // Create a unique request context for this request.
   // It will be passed between queues / threads (where implemented),
   // so it should be valid for the lifetime of the request.
@@ -133,9 +131,11 @@ class RequestContext {
       std::unique_ptr<RequestData>& data,
       bool strict);
 
-  struct State;
-  std::atomic<State*> state_;
-  std::mutex m_;
+  struct State {
+    std::map<std::string, std::unique_ptr<RequestData>> requestData_;
+    std::set<RequestData*> callbackData_;
+  };
+  folly::Synchronized<State> state_;
 };
 
 class RequestContextScopeGuard {