From: Marc Celani <marccelani@fb.com>
Date: Sat, 4 Mar 2017 06:27:50 +0000 (-0800)
Subject: Remove a RequestContext deadlock
X-Git-Tag: v2017.03.06.00~3
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=00eb2ce00f68bdb274844f1c6bb29ca6b8bbf6cf;p=folly.git

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
---

diff --git a/folly/io/async/Request.cpp b/folly/io/async/Request.cpp
index 8310033b..0c56d200 100644
--- a/folly/io/async/Request.cpp
+++ b/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(
diff --git a/folly/io/async/Request.h b/folly/io/async/Request.h
index fb4c5692..77243e12 100644
--- a/folly/io/async/Request.h
+++ b/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() {}
 };
diff --git a/folly/io/async/test/RequestContextTest.cpp b/folly/io/async/test/RequestContextTest.cpp
index 0ca773c3..ac24c5b5 100644
--- a/folly/io/async/test/RequestContextTest.cpp
+++ b/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");
+}