From: Pingjia Shan <pingjia@fb.com>
Date: Tue, 28 Nov 2017 06:10:59 +0000 (-0800)
Subject: Reset context shared_ptr in AsyncTimeout::cancelTimeout()
X-Git-Tag: v2017.12.04.00~29
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=acbd746f5036bf795cc62022f0114b4b08032d82;p=folly.git

Reset context shared_ptr in AsyncTimeout::cancelTimeout()

Summary:
This seems to fix issue in the attached task.

`context_` is set in `scheduleTimeout()` and never gets reset. So when an AsyncTimeout object is held across requests by objects like `IOThreadPool0` in I/O Thread Pool used by wangle/acceptor/ConnectionManager, RequestContext object created for the last request is leaked until `IOThreadPool0` handles another request and overrides `context_`. In the issue described in attached task, unit test has single request, next request never comes in, RequestContext doesn't end until test service stops, logging feature relying on RequestContext dtor doesn't get called in time, and thus unit test fails.

Reviewed By: yfeldblum

Differential Revision: D6402268

fbshipit-source-id: 200c6d358dfa6d7d9aa68ab05f6f1c7f4117b0ec
---

diff --git a/folly/io/async/AsyncTimeout.cpp b/folly/io/async/AsyncTimeout.cpp
index 680d64c1..ad5cae4b 100644
--- a/folly/io/async/AsyncTimeout.cpp
+++ b/folly/io/async/AsyncTimeout.cpp
@@ -90,6 +90,7 @@ bool AsyncTimeout::scheduleTimeout(uint32_t milliseconds) {
 void AsyncTimeout::cancelTimeout() {
   if (isScheduled()) {
     timeoutManager_->cancelTimeout(this);
+    context_.reset();
   }
 }
 
diff --git a/folly/io/async/test/AsyncTimeoutTest.cpp b/folly/io/async/test/AsyncTimeoutTest.cpp
index 1d7c04fe..3b16709a 100644
--- a/folly/io/async/test/AsyncTimeoutTest.cpp
+++ b/folly/io/async/test/AsyncTimeoutTest.cpp
@@ -77,8 +77,18 @@ TEST(AsyncTimeout, cancel_make) {
     [&]() noexcept { value = expected; }
   );
 
-  observer->scheduleTimeout(std::chrono::milliseconds(100));
-  observer->cancelTimeout();
+  std::weak_ptr<RequestContext> rctx_weak_ptr;
+
+  {
+    RequestContextScopeGuard rctx_guard;
+    rctx_weak_ptr = RequestContext::saveContext();
+    observer->scheduleTimeout(std::chrono::milliseconds(100));
+    observer->cancelTimeout();
+  }
+
+  // Ensure that RequestContext created for the scope has been released and
+  // deleted.
+  EXPECT_EQ(rctx_weak_ptr.expired(), true);
 
   manager.loop();
 
@@ -89,14 +99,24 @@ TEST(AsyncTimeout, cancel_schedule) {
   int value = 0;
   int const expected = 10;
   EventBase manager;
+  std::unique_ptr<AsyncTimeout> observer;
+  std::weak_ptr<RequestContext> rctx_weak_ptr;
 
-  auto observer = AsyncTimeout::schedule(
-    std::chrono::milliseconds(100),
-    manager,
-    [&]() noexcept { value = expected; }
-  );
+  {
+    RequestContextScopeGuard rctx_guard;
+    rctx_weak_ptr = RequestContext::saveContext();
+
+    observer = AsyncTimeout::schedule(
+        std::chrono::milliseconds(100), manager, [&]() noexcept {
+          value = expected;
+        });
+
+    observer->cancelTimeout();
+  }
 
-  observer->cancelTimeout();
+  // Ensure that RequestContext created for the scope has been released and
+  // deleted.
+  EXPECT_EQ(rctx_weak_ptr.expired(), true);
 
   manager.loop();