From 1c6da6e3d2f375d0e9ef063bd9c5ffcb0d07015d Mon Sep 17 00:00:00 2001 From: Jon Maltiel Swenson Date: Tue, 12 Sep 2017 21:30:08 -0700 Subject: [PATCH] Backed out changeset ea9041ce2280 Summary: Revert D5787837. Breaks `onSet()`/`onUnset()` behavior. Reviewed By: palmtenor Differential Revision: D5817063 fbshipit-source-id: c7dea636fa60eb616d4ebe0a9d418bc96b3018ae --- folly/fibers/FiberManagerInternal-inl.h | 16 +--- folly/fibers/test/FibersTest.cpp | 102 --------------------- folly/io/async/Request.cpp | 48 ++-------- folly/io/async/Request.h | 17 ---- folly/io/async/test/RequestContextTest.cpp | 43 --------- 5 files changed, 11 insertions(+), 215 deletions(-) diff --git a/folly/fibers/FiberManagerInternal-inl.h b/folly/fibers/FiberManagerInternal-inl.h index af225a12..20aa1d9d 100644 --- a/folly/fibers/FiberManagerInternal-inl.h +++ b/folly/fibers/FiberManagerInternal-inl.h @@ -113,6 +113,7 @@ inline void FiberManager::runReadyFiber(Fiber* fiber) { fiber->state_ == Fiber::NOT_STARTED || fiber->state_ == Fiber::READY_TO_RUN); currentFiber_ = fiber; + fiber->rcontext_ = RequestContext::setContext(std::move(fiber->rcontext_)); if (observer_) { observer_->starting(reinterpret_cast(fiber)); } @@ -138,6 +139,7 @@ inline void FiberManager::runReadyFiber(Fiber* fiber) { observer_->stopped(reinterpret_cast(fiber)); } currentFiber_ = nullptr; + fiber->rcontext_ = RequestContext::setContext(std::move(fiber->rcontext_)); } else if (fiber->state_ == Fiber::INVALID) { assert(fibersActive_ > 0); --fibersActive_; @@ -159,6 +161,7 @@ inline void FiberManager::runReadyFiber(Fiber* fiber) { observer_->stopped(reinterpret_cast(fiber)); } currentFiber_ = nullptr; + fiber->rcontext_ = RequestContext::setContext(std::move(fiber->rcontext_)); fiber->localData_.reset(); fiber->rcontext_.reset(); @@ -176,6 +179,7 @@ inline void FiberManager::runReadyFiber(Fiber* fiber) { observer_->stopped(reinterpret_cast(fiber)); } currentFiber_ = nullptr; + fiber->rcontext_ = RequestContext::setContext(std::move(fiber->rcontext_)); fiber->state_ = Fiber::READY_TO_RUN; yieldedFibers_.push_back(*fiber); } @@ -196,20 +200,8 @@ inline void FiberManager::loopUntilNoReadyImpl() { auto originalFiberManager = this; std::swap(currentFiberManager_, originalFiberManager); - RequestContext::Provider oldRequestContextProvider; - auto newRequestContextProvider = - [this, &oldRequestContextProvider]() -> std::shared_ptr& { - return currentFiber_ ? currentFiber_->rcontext_ - : oldRequestContextProvider(); - }; - oldRequestContextProvider = RequestContext::setRequestContextProvider( - std::ref(newRequestContextProvider)); - SCOPE_EXIT { isLoopScheduled_ = false; - // Restore RequestContext provider before call to ensureLoopScheduled() - RequestContext::setRequestContextProvider( - std::move(oldRequestContextProvider)); if (!readyFibers_.empty()) { ensureLoopScheduled(); } diff --git a/folly/fibers/test/FibersTest.cpp b/folly/fibers/test/FibersTest.cpp index 55caeb4b..73e9b6d6 100644 --- a/folly/fibers/test/FibersTest.cpp +++ b/folly/fibers/test/FibersTest.cpp @@ -33,7 +33,6 @@ #include #include #include -#include #include #include @@ -1237,107 +1236,6 @@ TEST(FiberManager, fiberLocalDestructor) { EXPECT_FALSE(fm.hasTasks()); } -TEST(FiberManager, fiberRequestContext) { - folly::EventBase evb; - FiberManager fm(std::make_unique()); - dynamic_cast(fm.loopController()) - .attachEventBase(evb); - - struct TestContext : public folly::RequestData { - explicit TestContext(std::string s) : data(std::move(s)) {} - std::string data; - }; - - class AfterFibersCallback : public folly::EventBase::LoopCallback { - public: - AfterFibersCallback( - folly::EventBase& evb, - const bool& fibersDone, - folly::Function afterFibersFunc) - : evb_(evb), - fibersDone_(fibersDone), - afterFibersFunc_(std::move(afterFibersFunc)) {} - - void runLoopCallback() noexcept override { - if (fibersDone_) { - afterFibersFunc_(); - delete this; - } else { - evb_.runInLoop(this); - } - } - - private: - folly::EventBase& evb_; - const bool& fibersDone_; - folly::Function afterFibersFunc_; - }; - - bool fibersDone = false; - size_t tasksRun = 0; - evb.runInEventBaseThread([&evb, &fm, &tasksRun, &fibersDone]() { - ++tasksRun; - auto* const evbCtx = folly::RequestContext::get(); - EXPECT_NE(nullptr, evbCtx); - EXPECT_EQ(nullptr, evbCtx->getContextData("key")); - evbCtx->setContextData("key", std::make_unique("evb_value")); - - // This callback allows us to check that FiberManager has restored the - // RequestContext provider as expected after a fiber loop. - auto* afterFibersCallback = - new AfterFibersCallback(evb, fibersDone, [&tasksRun, evbCtx]() { - ++tasksRun; - EXPECT_EQ(evbCtx, folly::RequestContext::get()); - EXPECT_EQ( - "evb_value", - dynamic_cast(evbCtx->getContextData("key"))->data); - }); - evb.runInLoop(afterFibersCallback); - - // Launching a fiber allows us to hit FiberManager RequestContext - // setup/teardown logic. - fm.addTask([&evb, &tasksRun, &fibersDone, evbCtx]() { - ++tasksRun; - - // Initially, fiber starts with same RequestContext as its parent task. - EXPECT_EQ(evbCtx, folly::RequestContext::get()); - EXPECT_NE(nullptr, evbCtx->getContextData("key")); - EXPECT_EQ( - "evb_value", - dynamic_cast(evbCtx->getContextData("key"))->data); - - // Create a new RequestContext for this fiber so we can distinguish from - // RequestContext first EventBase callback started with. - folly::RequestContext::create(); - auto* const fiberCtx = folly::RequestContext::get(); - EXPECT_NE(nullptr, fiberCtx); - EXPECT_EQ(nullptr, fiberCtx->getContextData("key")); - fiberCtx->setContextData( - "key", std::make_unique("fiber_value")); - - // Task launched from within fiber should share current fiber's - // RequestContext - evb.runInEventBaseThread([&tasksRun, fiberCtx]() { - ++tasksRun; - auto* const evbCtx2 = folly::RequestContext::get(); - EXPECT_EQ(fiberCtx, evbCtx2); - EXPECT_NE(nullptr, evbCtx2->getContextData("key")); - EXPECT_EQ( - "fiber_value", - dynamic_cast(evbCtx2->getContextData("key"))->data); - }); - - fibersDone = true; - }); - }); - - evb.loop(); - - EXPECT_EQ(4, tasksRun); - EXPECT_TRUE(fibersDone); - EXPECT_FALSE(fm.hasTasks()); -} - TEST(FiberManager, yieldTest) { FiberManager manager(std::make_unique()); auto& loopController = diff --git a/folly/io/async/Request.cpp b/folly/io/async/Request.cpp index a3bd7382..11894b5d 100644 --- a/folly/io/async/Request.cpp +++ b/folly/io/async/Request.cpp @@ -13,17 +13,14 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include -#include -#include -#include +#include +#include #include #include #include -#include namespace folly { @@ -118,50 +115,19 @@ std::shared_ptr RequestContext::setContext( return ctx; } -RequestContext::Provider& RequestContext::requestContextProvider() { - class DefaultProvider { - public: - constexpr DefaultProvider() = default; - DefaultProvider(const DefaultProvider&) = delete; - DefaultProvider& operator=(const DefaultProvider&) = delete; - DefaultProvider(DefaultProvider&&) = default; - DefaultProvider& operator=(DefaultProvider&&) = default; - - std::shared_ptr& operator()() { - return context; - } - - private: - std::shared_ptr context; - }; - - static SingletonThreadLocal providerSingleton( - []() { return new Provider(DefaultProvider()); }); - return providerSingleton.get(); -} - std::shared_ptr& RequestContext::getStaticContext() { - auto& provider = requestContextProvider(); - return provider(); + using SingletonT = SingletonThreadLocal>; + static SingletonT singleton; + + return singleton.get(); } RequestContext* RequestContext::get() { - auto& context = getStaticContext(); + auto context = getStaticContext(); if (!context) { static RequestContext defaultContext; return std::addressof(defaultContext); } return context.get(); } - -RequestContext::Provider RequestContext::setRequestContextProvider( - RequestContext::Provider newProvider) { - if (!newProvider) { - throw std::runtime_error("RequestContext provider must be non-empty"); - } - - auto& provider = requestContextProvider(); - std::swap(provider, newProvider); - return newProvider; -} } diff --git a/folly/io/async/Request.h b/folly/io/async/Request.h index 78762cf9..a4b7a23a 100644 --- a/folly/io/async/Request.h +++ b/folly/io/async/Request.h @@ -19,7 +19,6 @@ #include #include -#include #include #include @@ -46,8 +45,6 @@ class RequestContext; // copied between threads. class RequestContext { public: - using Provider = folly::Function&()>; - // Create a unique request context for this request. // It will be passed between queues / threads (where implemented), // so it should be valid for the lifetime of the request. @@ -98,22 +95,8 @@ class RequestContext { return getStaticContext(); } - // This API allows one to override the default behavior of getStaticContext() - // by providing a custom RequestContext provider. The old provider is - // returned, and the user must restore the old provider via a subsequent call - // to setRequestContextProvider() once the new provider is no longer needed. - // - // Using custom RequestContext providers can be more efficient than having to - // setContext() whenever context must be switched. This is especially true in - // applications that do not actually use RequestContext, but where library - // code must still support RequestContext for other use cases. See - // FiberManager for an example of how a custom RequestContext provider can - // reduce calls to setContext(). - static Provider setRequestContextProvider(Provider f); - private: static std::shared_ptr& getStaticContext(); - static Provider& requestContextProvider(); using Data = std::map>; folly::Synchronized data_; diff --git a/folly/io/async/test/RequestContextTest.cpp b/folly/io/async/test/RequestContextTest.cpp index 2ae4123b..750ff7fa 100644 --- a/folly/io/async/test/RequestContextTest.cpp +++ b/folly/io/async/test/RequestContextTest.cpp @@ -77,49 +77,6 @@ TEST(RequestContext, SimpleTest) { EXPECT_TRUE(nullptr != RequestContext::get()); } -TEST(RequestContext, nonDefaultContextsAreThreadLocal) { - RequestContext* ctx1 = nullptr; - RequestContext* ctx2 = nullptr; - - std::vector ts; - for (size_t i = 0; i < 2; ++i) { - auto*& ctx = (i == 0 ? ctx1 : ctx2); - ts.emplace_back([&ctx]() { - RequestContext::create(); - ctx = RequestContext::get(); - }); - } - for (auto& t : ts) { - t.join(); - } - - EXPECT_NE(nullptr, ctx1); - EXPECT_NE(nullptr, ctx2); - EXPECT_NE(ctx1, ctx2); -} - -TEST(RequestContext, customRequestContextProvider) { - auto customContext = std::make_shared(); - auto customProvider = [&customContext]() -> std::shared_ptr& { - return customContext; - }; - - auto* const originalContext = RequestContext::get(); - EXPECT_NE(nullptr, originalContext); - - // Install new RequestContext provider - auto originalProvider = - RequestContext::setRequestContextProvider(std::move(customProvider)); - - auto* const newContext = RequestContext::get(); - EXPECT_EQ(customContext.get(), newContext); - EXPECT_NE(originalContext, newContext); - - // Restore original RequestContext provider - RequestContext::setRequestContextProvider(std::move(originalProvider)); - EXPECT_EQ(originalContext, RequestContext::get()); -} - TEST(RequestContext, setIfAbsentTest) { EXPECT_TRUE(RequestContext::get() != nullptr); -- 2.34.1