Fix RequestContext held too long issue in EventBase
authorPingjia Shan <pingjia@fb.com>
Tue, 5 Dec 2017 21:03:44 +0000 (13:03 -0800)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Tue, 5 Dec 2017 21:06:41 +0000 (13:06 -0800)
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

folly/io/async/EventBase.cpp
folly/io/async/EventBase.h
folly/io/async/test/EventBaseTest.cpp

index 340b210..c1ed733 100644 (file)
@@ -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();
     }
 
index e688abe..3399cea 100644 (file)
@@ -150,6 +150,7 @@ class EventBase : private boost::noncopyable,
 
     virtual void runLoopCallback() noexcept = 0;
     void cancelLoopCallback() {
+      context_.reset();
       unlink();
     }
 
index 25b3192..f8b2b1e 100644 (file)
@@ -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());
 }