Skip to content

Commit

Permalink
MB-45378: Fix static init fiasco with ExecutorPool & GoogleLog
Browse files Browse the repository at this point in the history
When converting platform to be statically linked, a crash is seen
during shutdown of ep-engine_ep_unit_tests.DcpConnMapTest tests on
MSVC. The ExecutorPool is consuming messages on the background threads
(I believe to coordinate shutdown), and during that it attempts to log
a warning message to Google Log.

The cause of the crash is a change in the static initialisation (and
deinitialization) order - the GoogleLog singleton instance as used
internally by Folly is deinitialized before ExecutorPool singleton. As
such, when the ExecutorPool singleton is shutting down, it attempts to
log a message to a non-existant GLog instance and a nullptr is
deferenced.

Fix by changing ExecutorPool singleton to use C++11 magic static
(Meyer singleton); which ensures it is destructed earlier, before
GLog.

Additionally, while the above is sufficient to fix this issue on macOS
Catalina, on Mojave this introduces _another_ crash as some Folly
hazard pointer singletons appear to already have been destructed and
the following crash is seen:

    * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
      * frame #0: 0x00007fff7412f2c6 libsystem_kernel.dylib`__pthread_kill + 10
        frame #1: 0x00007fff741eabf1 libsystem_pthread.dylib`pthread_kill + 284
        frame #2: 0x00007fff740996a6 libsystem_c.dylib`abort + 127
        frame #3: 0x00007fff741a8077 libsystem_malloc.dylib`malloc_vreport + 545
        frame #4: 0x00007fff741a7e38 libsystem_malloc.dylib`malloc_report + 151
        frame #5: 0x00007fff73ff3cf9 libdyld.dylib`_tlv_atexit + 155
        frame #6: 0x000000010143cb2d ep-engine_ep_unit_tests`folly::SingletonThreadLocal<folly::hazptr_tc<std::__1::atomic>, folly::hazptr_tc_tls_tag, folly::detail::DefaultMake<folly::hazptr_tc<std::__1::atomic> >, folly::hazptr_tc_tls_tag>::getSlow(cache=0x000000010b5606b8) at SingletonThreadLocal.h:157 [opt]
        frame #7: 0x0000000101437a19 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(folly::CPUThreadPoolExecutor::CPUTask) [inlined] folly::SingletonThreadLocal<folly::hazptr_tc<std::__1::atomic>, folly::hazptr_tc_tls_tag, folly::detail::DefaultMake<folly::hazptr_tc<std::__1::atomic> >, folly::hazptr_tc_tls_tag>::get() at SingletonThreadLocal.h:167 [opt]
        frame #8: 0x0000000101437a08 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(folly::CPUThreadPoolExecutor::CPUTask) [inlined] folly::hazptr_tc<std::__1::atomic>& folly::hazptr_tc_tls<std::__1::atomic>() at HazptrThrLocal.h:166 [opt]
        frame #9: 0x0000000101437a08 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(folly::CPUThreadPoolExecutor::CPUTask) at HazptrHolder.h:64 [opt]
        frame #10: 0x0000000101437a08 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(folly::CPUThreadPoolExecutor::CPUTask) [inlined] folly::hazptr_holder<std::__1::atomic>::hazptr_holder(this=<unavailable>, domain=<unavailable>) at HazptrHolder.h:61 [opt]
        frame #11: 0x0000000101437a08 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(folly::CPUThreadPoolExecutor::CPUTask) at UnboundedQueue.h:374 [opt]
        frame #12: 0x00000001014379e7 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(folly::CPUThreadPoolExecutor::CPUTask) [inlined] folly::UnboundedQueue<folly::CPUThreadPoolExecutor::CPUTask, false, false, false, 6ul, 7ul, std::__1::atomic>::enqueue(this=0x00007ffeefbff770, arg=0x00007ffeefbff690) at UnboundedQueue.h:271 [opt]
        frame #13: 0x00000001014379e7 ep-engine_ep_unit_tests`folly::UnboundedBlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::add(this=0x000000010ba00f80, item=CPUTask @ 0x00007ffeefbff690) at UnboundedBlockingQueue.h:31 [opt]
        frame #14: 0x0000000101437bfc ep-engine_ep_unit_tests`folly::BlockingQueue<folly::CPUThreadPoolExecutor::CPUTask>::addWithPriority(this=0x000000010ba00f80, item=CPUTask @ 0x00007ffeefbff770, (null)=<unavailable>) at BlockingQueue.h:57 [opt]
        frame #15: 0x0000000101436b00 ep-engine_ep_unit_tests`folly::CPUThreadPoolExecutor::stopThreads(this=0x000000010bf8de00, n=2) at CPUThreadPoolExecutor.cpp:281 [opt]
        frame #16: 0x000000010144bae3 ep-engine_ep_unit_tests`folly::ThreadPoolExecutor::stop() [inlined] folly::ThreadPoolExecutor::removeThreads(this=<unavailable>, n=<unavailable>) at ThreadPoolExecutor.cpp:233 [opt]
        frame #17: 0x000000010144bad0 ep-engine_ep_unit_tests`folly::ThreadPoolExecutor::stop(this=0x000000010bf8de00) at ThreadPoolExecutor.cpp:251 [opt]
        frame #18: 0x00000001014352d4 ep-engine_ep_unit_tests`folly::CPUThreadPoolExecutor::~CPUThreadPoolExecutor(this=0x000000010bf8de00, vtt=0x00000001019fd6c8) at CPUThreadPoolExecutor.cpp:126 [opt]
        frame #19: 0x0000000101435465 ep-engine_ep_unit_tests`folly::CPUThreadPoolExecutor::~CPUThreadPoolExecutor() [inlined] folly::CPUThreadPoolExecutor::~CPUThreadPoolExecutor(this=0x000000010bf8de00) at CPUThreadPoolExecutor.cpp:124 [opt]
        frame #20: 0x0000000101435459 ep-engine_ep_unit_tests`folly::CPUThreadPoolExecutor::~CPUThreadPoolExecutor(this=0x000000010bf8de00) at CPUThreadPoolExecutor.cpp:124 [opt]
        frame #21: 0x000000010023240a ep-engine_ep_unit_tests`FollyExecutorPool::~FollyExecutorPool(this=0x000000010b7eed40) at folly_executorpool.cc:757 [opt]
        frame #22: 0x00000001002325ee ep-engine_ep_unit_tests`FollyExecutorPool::~FollyExecutorPool(this=0x000000010b7eed40) at folly_executorpool.cc:751 [opt]
        frame #23: 0x00007fff7409a3cf libsystem_c.dylib`__cxa_finalize_ranges + 319
        frame #24: 0x00007fff7409a6b3 libsystem_c.dylib`exit + 55

Address _this_ with a somewhat belt-and-braces approach - also
manually shutdown the ExecutorPool in DcpConnMapTest::TearDown - as is
done in other tests.

Change-Id: I87f13bc3a7cdf616b52d18502dd724fcf630d3b9
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/152230
Tested-by: Build Bot <[email protected]>
Reviewed-by: Richard de Mellow <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>
  • Loading branch information
daverigby authored and trondn committed Apr 29, 2021
1 parent 5b78cd9 commit 5c90329
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 18 deletions.
20 changes: 10 additions & 10 deletions engines/ep/src/executorpool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,17 @@
#include "objectregistry.h"

std::mutex ExecutorPool::initGuard;
std::atomic<ExecutorPool*> ExecutorPool::instance;

static const size_t EP_MIN_NONIO_THREADS = 2;

static const size_t EP_MAX_AUXIO_THREADS = 8;
static const size_t EP_MAX_NONIO_THREADS = 8;

ExecutorPool *ExecutorPool::get() {
auto* tmp = instance.load();
auto* tmp = getInstance().get();
if (tmp == nullptr) {
LockHolder lh(initGuard);
tmp = instance.load();
tmp = getInstance().get();
if (tmp == nullptr) {
// Double-checked locking if instance is null - ensure two threads
// don't both create an instance.
Expand Down Expand Up @@ -58,20 +57,16 @@ ExecutorPool *ExecutorPool::get() {
"ExecutorPool::get() Invalid executor_pool_backend '" +
config.getExecutorPoolBackend() + "'");
}
instance.store(tmp);
getInstance().reset(tmp);
}
}
return tmp;
}

void ExecutorPool::shutdown() {
std::lock_guard<std::mutex> lock(initGuard);
auto* tmp = instance.load();
if (tmp != nullptr) {
NonBucketAllocationGuard guard;
delete tmp;
instance = nullptr;
}
NonBucketAllocationGuard guard;
getInstance().reset();
}

ExecutorPool::ExecutorPool(size_t maxThreads)
Expand Down Expand Up @@ -213,3 +208,8 @@ int ExecutorPool::getThreadPriority(task_type_t taskType) {
#endif
return 0;
}

std::unique_ptr<ExecutorPool>& ExecutorPool::getInstance() {
static std::unique_ptr<ExecutorPool> instance;
return instance;
}
4 changes: 3 additions & 1 deletion engines/ep/src/executorpool.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,11 @@ class ExecutorPool {
*/
size_t calcNumNonIO(size_t threadCount) const;

// Return a reference to the singleton ExecutorPool.
static std::unique_ptr<ExecutorPool>& getInstance();

// Singleton creation
static std::mutex initGuard;
static std::atomic<ExecutorPool*> instance;

/**
* Maximum number of threads of any given class (Reader, Writer, AuxIO,
Expand Down
9 changes: 5 additions & 4 deletions engines/ep/src/fakes/fake_executorpool.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

#include <folly/portability/GTest.h>

#include <memory>

class SingleThreadedExecutorPool : public CB3ExecutorPool {
public:

Expand All @@ -39,17 +41,16 @@ class SingleThreadedExecutorPool : public CB3ExecutorPool {
*/
static void replaceExecutorPoolWithFake() {
LockHolder lh(initGuard);
auto* tmp = instance.load();
if (tmp != nullptr) {
auto& instance = getInstance();
if (instance) {
throw std::runtime_error("replaceExecutorPoolWithFake: "
"ExecutorPool instance already created - cowardly refusing to continue!");
}

EventuallyPersistentEngine *epe =
ObjectRegistry::onSwitchThread(nullptr, true);
tmp = new SingleThreadedExecutorPool();
instance = std::make_unique<SingleThreadedExecutorPool>();
ObjectRegistry::onSwitchThread(epe);
instance.store(tmp);
}

explicit SingleThreadedExecutorPool()
Expand Down
7 changes: 4 additions & 3 deletions engines/ep/tests/mock/mock_executor_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,20 @@
*/

#include "mock_executor_pool.h"

#include "objectregistry.h"
#include "taskqueue.h"
#include <memory>

void MockExecutorPool::replaceExecutorPoolWithMock() {
LockHolder lh(initGuard);
auto* executor = instance.load();
auto& executor = getInstance();
if (executor) {
executor->shutdown();
}
auto* epEngine = ObjectRegistry::onSwitchThread(nullptr, true);
executor = new MockExecutorPool();
executor = std::make_unique<MockExecutorPool>();
ObjectRegistry::onSwitchThread(epEngine);
instance.store(executor);
}

bool MockExecutorPool::isTaskScheduled(const task_type_t queueType,
Expand Down
2 changes: 2 additions & 0 deletions engines/ep/tests/module_tests/dcp_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1700,7 +1700,9 @@ class DcpConnMapTest : public ::testing::Test {
}

void TearDown() override {
engine.reset();
ObjectRegistry::onSwitchThread(nullptr);
ExecutorPool::shutdown();
}

/**
Expand Down

0 comments on commit 5c90329

Please sign in to comment.