Commit bda67fde authored by Pingjia Shan's avatar Pingjia Shan Committed by Facebook Github Bot

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
parent 5a07e203
...@@ -612,7 +612,7 @@ bool EventBase::runLoopCallbacks() { ...@@ -612,7 +612,7 @@ bool EventBase::runLoopCallbacks() {
while (!currentCallbacks.empty()) { while (!currentCallbacks.empty()) {
LoopCallback* callback = &currentCallbacks.front(); LoopCallback* callback = &currentCallbacks.front();
currentCallbacks.pop_front(); currentCallbacks.pop_front();
folly::RequestContextScopeGuard rctx(callback->context_); folly::RequestContextScopeGuard rctx(std::move(callback->context_));
callback->runLoopCallback(); callback->runLoopCallback();
} }
......
...@@ -150,6 +150,7 @@ class EventBase : private boost::noncopyable, ...@@ -150,6 +150,7 @@ class EventBase : private boost::noncopyable,
virtual void runLoopCallback() noexcept = 0; virtual void runLoopCallback() noexcept = 0;
void cancelLoopCallback() { void cancelLoopCallback() {
context_.reset();
unlink(); unlink();
} }
......
...@@ -1414,7 +1414,7 @@ TEST(EventBaseTest, CancelRunInLoop) { ...@@ -1414,7 +1414,7 @@ TEST(EventBaseTest, CancelRunInLoop) {
// Run the loop // Run the loop
eventBase.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 // stopped re-installing themselves
ASSERT_EQ(cancelC1.getCount(), 0); ASSERT_EQ(cancelC1.getCount(), 0);
ASSERT_EQ(cancelC2.getCount(), 0); ASSERT_EQ(cancelC2.getCount(), 0);
...@@ -1914,15 +1914,44 @@ TEST(EventBaseTest, DrivableExecutorTest) { ...@@ -1914,15 +1914,44 @@ TEST(EventBaseTest, DrivableExecutorTest) {
TEST(EventBaseTest, RequestContextTest) { TEST(EventBaseTest, RequestContextTest) {
EventBase evb; EventBase evb;
auto defaultCtx = RequestContext::get(); auto defaultCtx = RequestContext::get();
std::weak_ptr<RequestContext> rctx_weak_ptr;
{ {
RequestContextScopeGuard rctx; RequestContextScopeGuard rctx;
rctx_weak_ptr = RequestContext::saveContext();
auto context = RequestContext::get(); auto context = RequestContext::get();
EXPECT_NE(defaultCtx, context); EXPECT_NE(defaultCtx, context);
evb.runInLoop([context] { EXPECT_EQ(context, RequestContext::get()); }); 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()); 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()); EXPECT_EQ(defaultCtx, RequestContext::get());
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment