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 8310033b2020529c1a9f16453f44c0a521a27455..0c56d200aaec9cb0c333f12a14f1488dd9011122 100644 (file)
--- 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 fb4c5692e2f43db367325d1b4686d72692d2a9db..77243e123fbbf7a09ce9e921988e0c36f5e221bb 100644 (file)
--- 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 0ca773c3caeb33126e779aaff5577dfdea05196f..ac24c5b5e6ca55da8146e0bd7c9de2ca618578d5 100644 (file)
--- 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");
+}