From acbd746f5036bf795cc62022f0114b4b08032d82 Mon Sep 17 00:00:00 2001
From: Pingjia Shan <pingjia@fb.com>
Date: Mon, 27 Nov 2017 22:10:59 -0800
Subject: [PATCH] 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
---
 folly/io/async/AsyncTimeout.cpp          |  1 +
 folly/io/async/test/AsyncTimeoutTest.cpp | 36 ++++++++++++++++++------
 2 files changed, 29 insertions(+), 8 deletions(-)

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();
 
-- 
2.34.1