Remove a RequestContext deadlock

Summary:
It is not unusual to write a RequestData that will send a request to a backend service upon destruction (for example, to log the request data). This typically involves calling EventBase::runInEventBaseThread() to do the logging. Normally, if you wait until the RequestContext is destructing for this to happen, everything works fine. However, if you explicitly call clearContextData(), then this can cause a deadlock. clearContextData() holds the write lock on the data_. Imagine the NotificationQueue thread tries to set the context (for some other reason), and it is waiting on the read lock on the data_. It does so while holding a lock on its own queue. Now, the clearContextData() calls the destructor while holding the write lock on data_, and the destructor calls runInEventBaseThread, which grabs the queue's lock. Deadlock.

This can either be fixed in NotificationQueue or RequestContext. I decided to fix it in RequestContext because there may be other deadlocks that don't involve NotificationQueue (see the test plan).

There are other deadlocks in RequestContext if you call write methods in RequestData::onSet or onUnset. But fixing this would require a massive perf regression (I imagine using shared_ptr instead of unique_ptr, and copying the ptrs under readlock and calling the callbacks after releasing the lock - yuck!) So instead, warn people against doing that.

Reviewed By: yfeldblum

Differential Revision: D4655400

fbshipit-source-id: cfebe696953a19dc4dba49976bbcd0ec1363bc42

folly/io/async/Request.cpp

@@ -88,7 +88,17 @@ void RequestContext::onUnset() {
 }
 
 void RequestContext::clearContextData(const std::string& val) {
-  data_.wlock()->erase(val);
+  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.
+  {
+    auto wlock = data_.wlock();
+    auto it = wlock->find(val);
+    if (it != wlock->end()) {
+      requestData = std::move(it->second);
+      wlock->erase(it);
+    }
+  }
 }
 
 std::shared_ptr<RequestContext> RequestContext::setContext(

folly/io/async/Request.h

@@ -34,6 +34,10 @@ namespace folly {
 class RequestData {
  public:
   virtual ~RequestData() = default;
+  // Avoid calling RequestContext::setContextData, setContextDataIfAbsent, or
+  // clearContextData from these callbacks. Doing so will cause deadlock. We
+  // could fix these deadlocks, but only at significant performance penalty, so
+  // just don't do it!
   virtual void onSet() {}
   virtual void onUnset() {}
 };

folly/io/async/test/RequestContextTest.cpp

@@ -129,3 +129,25 @@ TEST(RequestContext, testSetUnset) {
   EXPECT_EQ(1, testData2->set_);
   EXPECT_EQ(1, testData2->unset_);
 }
+
+TEST(RequestContext, deadlockTest) {
+  class DeadlockTestData : public RequestData {
+   public:
+    explicit DeadlockTestData(const std::string& val) : val_(val) {}
+
+    virtual ~DeadlockTestData() {
+      RequestContext::get()->setContextData(
+          val_, folly::make_unique<TestData>(1));
+    }
+
+    void onSet() override {}
+
+    void onUnset() override {}
+
+    std::string val_;
+  };
+
+  RequestContext::get()->setContextData(
+      "test", folly::make_unique<DeadlockTestData>("test2"));
+  RequestContext::get()->clearContextData("test");
+}