From cfc602df8f8691d73fddea64f5bdc147a0f5c9b0 Mon Sep 17 00:00:00 2001 From: Giuseppe Ottaviano Date: Mon, 18 Dec 2017 08:40:40 -0800 Subject: [PATCH] Make global executors shutdown-safe Summary: The `get*Executor()` APIs don't check whether the singletons haven't been destroyed already. Add a check and allow to return `nullptr` during shutdown. Also do a general clean up of the code, there was no reason to use three independent singletons (non-atomically destroyed) for each executor. Reviewed By: philippv, luciang Differential Revision: D6589486 fbshipit-source-id: 20fb835db7e446bd811bbd6d5ddbc41db9e98b54 --- folly/executors/GlobalExecutor.cpp | 138 +++++++++++++++-------------- folly/executors/GlobalExecutor.h | 47 ++++++---- 2 files changed, 105 insertions(+), 80 deletions(-) diff --git a/folly/executors/GlobalExecutor.cpp b/folly/executors/GlobalExecutor.cpp index 0341e712..6fd66194 100644 --- a/folly/executors/GlobalExecutor.cpp +++ b/folly/executors/GlobalExecutor.cpp @@ -14,6 +14,11 @@ * limitations under the License. */ +#include +#include + +#include +#include #include #include #include @@ -23,90 +28,93 @@ using namespace folly; namespace { -// lock protecting global CPU executor -struct CPUExecutorLock {}; -Singleton globalCPUExecutorLock; -// global CPU executor -Singleton> globalCPUExecutor; -// default global CPU executor is an InlineExecutor -Singleton> globalInlineExecutor([] { - return new std::shared_ptr( - std::make_shared()); -}); - -// lock protecting global IO executor -struct IOExecutorLock {}; -Singleton globalIOExecutorLock; -// global IO executor -Singleton> globalIOExecutor; -// default global IO executor is an IOThreadPoolExecutor -Singleton> globalIOThreadPool([] { - return new std::shared_ptr( - std::make_shared( - sysconf(_SC_NPROCESSORS_ONLN), - std::make_shared("GlobalIOThreadPool"))); -}); -} // namespace - -namespace folly { +template +class GlobalExecutor { + public: + explicit GlobalExecutor( + Function()> constructDefault) + : constructDefault_(std::move(constructDefault)) {} + + std::shared_ptr get() { + { + SharedMutex::ReadHolder guard(mutex_); + if (auto executor = executor_.lock()) { + return executor; // Fast path. + } + } -template -std::shared_ptr getExecutor( - Singleton>& sExecutor, - Singleton>& sDefaultExecutor, - Singleton& sExecutorLock) { - std::shared_ptr executor; - auto singleton = sExecutor.try_get(); - auto lock = sExecutorLock.try_get(); - - { - RWSpinLock::ReadHolder guard(lock.get()); - if ((executor = sExecutor.try_get()->lock())) { + SharedMutex::WriteHolder guard(mutex_); + if (auto executor = executor_.lock()) { return executor; } + + if (!defaultExecutor_) { + defaultExecutor_ = constructDefault_(); + } + + return defaultExecutor_; } - RWSpinLock::WriteHolder guard(lock.get()); - executor = singleton->lock(); - if (!executor) { - std::weak_ptr defaultExecutor = *sDefaultExecutor.try_get().get(); - executor = defaultExecutor.lock(); - sExecutor.try_get().get()->swap(defaultExecutor); + void set(std::weak_ptr executor) { + SharedMutex::WriteHolder guard(mutex_); + executor_.swap(executor); } - return executor; -} -template -void setExecutor( - std::weak_ptr executor, - Singleton>& sExecutor, - Singleton& sExecutorLock) { - auto lock = sExecutorLock.try_get(); - RWSpinLock::WriteHolder guard(*lock); - std::weak_ptr executor_weak = std::move(executor); - sExecutor.try_get().get()->swap(executor_weak); -} + private: + SharedMutex mutex_; + std::weak_ptr executor_; + std::shared_ptr defaultExecutor_; + Function()> constructDefault_; +}; + +Singleton> gGlobalCPUExecutor([] { + return new GlobalExecutor( + // Default global CPU executor is an InlineExecutor. + [] { return std::make_unique(); }); +}); + +Singleton> gGlobalIOExecutor([] { + return new GlobalExecutor( + // Default global IO executor is an IOThreadPoolExecutor. + [] { + return std::make_unique( + std::thread::hardware_concurrency(), + std::make_shared("GlobalIOThreadPool")); + }); +}); + +} // namespace + +namespace folly { std::shared_ptr getCPUExecutor() { - return getExecutor( - globalCPUExecutor, globalInlineExecutor, globalCPUExecutorLock); + if (auto singleton = gGlobalCPUExecutor.try_get()) { + return singleton->get(); + } + return nullptr; } void setCPUExecutor(std::weak_ptr executor) { - setExecutor(std::move(executor), globalCPUExecutor, globalCPUExecutorLock); + if (auto singleton = gGlobalCPUExecutor.try_get()) { + singleton->set(std::move(executor)); + } } std::shared_ptr getIOExecutor() { - return getExecutor( - globalIOExecutor, globalIOThreadPool, globalIOExecutorLock); + if (auto singleton = gGlobalIOExecutor.try_get()) { + return singleton->get(); + } + return nullptr; } -EventBase* getEventBase() { - return getIOExecutor()->getEventBase(); +void setIOExecutor(std::weak_ptr executor) { + if (auto singleton = gGlobalIOExecutor.try_get()) { + singleton->set(std::move(executor)); + } } -void setIOExecutor(std::weak_ptr executor) { - setExecutor(std::move(executor), globalIOExecutor, globalIOExecutorLock); +EventBase* getEventBase() { + return getIOExecutor()->getEventBase(); } } // namespace folly diff --git a/folly/executors/GlobalExecutor.h b/folly/executors/GlobalExecutor.h index 79f5dce6..b145b595 100644 --- a/folly/executors/GlobalExecutor.h +++ b/folly/executors/GlobalExecutor.h @@ -23,27 +23,44 @@ namespace folly { -// Retrieve the global Executor. If there is none, a default InlineExecutor -// will be constructed and returned. This is named CPUExecutor to distinguish -// it from IOExecutor below and to hint that it's intended for CPU-bound tasks. +/** + * Retrieve the global Executor. If there is none, a default InlineExecutor + * will be constructed and returned. This is named CPUExecutor to distinguish + * it from IOExecutor below and to hint that it's intended for CPU-bound tasks. + * + * Can return nullptr on shutdown. + */ std::shared_ptr getCPUExecutor(); -// Set an Executor to be the global Executor which will be returned by -// subsequent calls to getCPUExecutor(). +/** + * Set an Executor to be the global Executor which will be returned by + * subsequent calls to getCPUExecutor(). + */ void setCPUExecutor(std::weak_ptr executor); -// Retrieve the global IOExecutor. If there is none, a default -// IOThreadPoolExecutor will be constructed and returned. -// -// IOExecutors differ from Executors in that they drive and provide access to -// one or more EventBases. +/** + * Retrieve the global IOExecutor. If there is none, a default + * IOThreadPoolExecutor will be constructed and returned. + * + * IOExecutors differ from Executors in that they drive and provide access to + * one or more EventBases. + * + * Can return nullptr on shutdown. + */ std::shared_ptr getIOExecutor(); -// Retrieve an event base from the global IOExecutor -folly::EventBase* getEventBase(); - -// Set an IOExecutor to be the global IOExecutor which will be returned by -// subsequent calls to getIOExecutor(). +/** + * Set an IOExecutor to be the global IOExecutor which will be returned by + * subsequent calls to getIOExecutor(). + */ void setIOExecutor(std::weak_ptr executor); +/** + * Retrieve an event base from the global IOExecutor + * + * NOTE: This is not shutdown-safe, the returned pointer may be + * invalid during shutdown. + */ +folly::EventBase* getEventBase(); + } // namespace folly -- 2.34.1