From: Pingjia Shan <pingjia@fb.com>
Date: Tue, 5 Dec 2017 21:03:44 +0000 (-0800)
Subject: Fix RequestContext held too long issue in EventBase
X-Git-Tag: v2017.12.11.00~25
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=bda67fde120837b77ddab74f23abcb22ae5b3029

Fix RequestContext held too long issue in EventBase

Summary:
Upon debugging for the attached task, it appears to me the problem are in two places:
. After a callback has been run, context_ wasn't reset
. After a callback has been canceled, context_ wasn't reset

In this diff:
. Fix these two places.
. Updating unit tests to cover these two cases.

Reviewed By: yfeldblum

Differential Revision: D6465788

fbshipit-source-id: 85b3b29dc80c9f3971c85f302385d41ded44fa0e
---

diff --git a/folly/io/async/EventBase.cpp b/folly/io/async/EventBase.cpp
index 340b2100..c1ed7336 100644
--- a/folly/io/async/EventBase.cpp
+++ b/folly/io/async/EventBase.cpp
@@ -612,7 +612,7 @@ bool EventBase::runLoopCallbacks() {
     while (!currentCallbacks.empty()) {
       LoopCallback* callback = &currentCallbacks.front();
       currentCallbacks.pop_front();
-      folly::RequestContextScopeGuard rctx(callback->context_);
+      folly::RequestContextScopeGuard rctx(std::move(callback->context_));
       callback->runLoopCallback();
     }
 
diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h
index e688abe7..3399cea2 100644
--- a/folly/io/async/EventBase.h
+++ b/folly/io/async/EventBase.h
@@ -150,6 +150,7 @@ class EventBase : private boost::noncopyable,
 
     virtual void runLoopCallback() noexcept = 0;
     void cancelLoopCallback() {
+      context_.reset();
       unlink();
     }
 
diff --git a/folly/io/async/test/EventBaseTest.cpp b/folly/io/async/test/EventBaseTest.cpp
index 25b3192c..f8b2b1e0 100644
--- a/folly/io/async/test/EventBaseTest.cpp
+++ b/folly/io/async/test/EventBaseTest.cpp
@@ -1414,7 +1414,7 @@ TEST(EventBaseTest, CancelRunInLoop) {
   // Run the loop
   eventBase.loop();
 
-  // cancelC1 and cancelC3 should have both fired after 10 iterations and
+  // cancelC1 and cancelC2 should have both fired after 10 iterations and
   // stopped re-installing themselves
   ASSERT_EQ(cancelC1.getCount(), 0);
   ASSERT_EQ(cancelC2.getCount(), 0);
@@ -1914,15 +1914,44 @@ TEST(EventBaseTest, DrivableExecutorTest) {
 TEST(EventBaseTest, RequestContextTest) {
   EventBase evb;
   auto defaultCtx = RequestContext::get();
+  std::weak_ptr<RequestContext> rctx_weak_ptr;
 
   {
     RequestContextScopeGuard rctx;
+    rctx_weak_ptr = RequestContext::saveContext();
     auto context = RequestContext::get();
     EXPECT_NE(defaultCtx, context);
     evb.runInLoop([context] { EXPECT_EQ(context, RequestContext::get()); });
+    evb.loop();
   }
 
+  // Ensure that RequestContext created for the scope has been released and
+  // deleted.
+  EXPECT_EQ(rctx_weak_ptr.expired(), true);
+
   EXPECT_EQ(defaultCtx, RequestContext::get());
-  evb.loop();
+}
+
+TEST(EventBaseTest, CancelLoopCallbackRequestContextTest) {
+  EventBase evb;
+  CountedLoopCallback c(&evb, 1);
+
+  auto defaultCtx = RequestContext::get();
+  EXPECT_EQ(defaultCtx, RequestContext::get());
+  std::weak_ptr<RequestContext> rctx_weak_ptr;
+
+  {
+    RequestContextScopeGuard rctx;
+    rctx_weak_ptr = RequestContext::saveContext();
+    auto context = RequestContext::get();
+    EXPECT_NE(defaultCtx, context);
+    evb.runInLoop(&c);
+    c.cancelLoopCallback();
+  }
+
+  // Ensure that RequestContext created for the scope has been released and
+  // deleted.
+  EXPECT_EQ(rctx_weak_ptr.expired(), true);
+
   EXPECT_EQ(defaultCtx, RequestContext::get());
 }