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 680d64c1f4ced91fef96c14f78b3a6d23c42cbf9..ad5cae4bfcad6772b30919e85c9a2755a8a5077b 100644 (file)
--- 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 1d7c04fe874b3fa60d6315336ca760c7429315bb..3b16709a84ad251f0c12b0cdc2f0a0e80f237e35 100644 (file)
--- 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();