Dead lock when adding child memory pool with already existing name

[zhztheplayer]

This causes the intended error Child memory pool {} already exists in {} never get raised.

Code that acquired lock:

velox/velox/common/memory/MemoryPool.cpp

Lines 349 to 355 in cc6ac91

	std::unique_lock guard{poolMutex_};
	VELOX_CHECK_EQ(
	children_.count(name),
	0,
	"Child memory pool {} already exists in {}",
	name,
	toString());

Code that awaits for lock:

velox/velox/common/memory/MemoryPool.cpp

Lines 276 to 288 in cc6ac91

	void MemoryPool::visitChildren(
	const std::function<bool(MemoryPool*)>& visitor) const {
	std::vector<std::shared_ptr<MemoryPool>> children;
	{
	std::shared_lock guard{poolMutex_};
	children.reserve(children_.size());
	for (auto& entry : children_) {
	auto child = entry.second.lock();
	if (child != nullptr) {
	children.push_back(std::move(child));
	}
	}
	}

Call stack:

syscall 0x00007f09e70f073d
folly::detail::nativeFutexWaitImpl Futex.cpp:126
folly::detail::futexWaitImpl Futex.cpp:254
folly::detail::futexWait<…> Futex-inl.h:94
folly::SharedMutexImpl::WaitForever::doWait SharedMutex.h:738
folly::SharedMutexImpl::futexWaitForZeroBits<…> SharedMutex.h:1191
folly::SharedMutexImpl::yieldWaitForZeroBits<…> SharedMutex.h:1158
folly::SharedMutexImpl::waitForZeroBits<…> SharedMutex.h:1122
folly::SharedMutexImpl::waitForZeroBits<…> SharedMutex.h:1111
folly::SharedMutexImpl::lockSharedImpl<…> SharedMutex.h:1486
folly::SharedMutexImpl::lockSharedImpl<…> SharedMutex.h:1359
folly::SharedMutexImpl::lock_shared SharedMutex.h:491
std::shared_lock::lock SharedMutex.h:1684
std::shared_lock::shared_lock SharedMutex.h:1634
facebook::velox::memory::MemoryPool::visitChildren(const std::function<…> &) const MemoryPool.cpp:280
facebook::velox::memory::MemoryPoolImpl::usedBytes MemoryPool.cpp:699
facebook::velox::memory::MemoryPoolImpl::toStringLocked[abi:cxx11]() const MemoryPool.h:944
facebook::velox::memory::MemoryPoolImpl::toString[abi:cxx11]() const MemoryPool.h:670
facebook::velox::memory::MemoryPool::addAggregateChild MemoryPool.cpp:350

[zhztheplayer]

cc @xiaoxmeng

[lingbin]

@zhztheplayer The call stack you gave is very helpful, and I feel like I already know the cause of the problem, I can write a cause analysis of this problem soon.

I can give a quick fix if you are not working on this bug.

In addition, I am curious: in what scenario would you try to add a child node with a duplicate name? Can you share a little bit?

[lingbin]

The root cause of this problem comes from MemoryPool::poolMutex_ (not MemoryPoolImpl::mutex_).

MemoryPool::poolMutex_ is used to protect MemoryPool::children_ in the following 4 member methods: addLeafChild()/addAggregateChild()/getChildCount()/visitChildren.

In other words, to avoid deadlock:

1. First of all, we can't call the above 4 methods while holding poolMutex_.
2. Furthermore, we cannot call methods that use these 4 methods while holding poolMutex_;
3. Finally, there cannot be any direct or indirect calls between these 4 methods. (This is the root cause of this problem).

addAggregateChild() ------ will acquire 'poolMutex_'
--> toString() [When duplicate names appear, used to print error messages]
    --> usedBytes()
        --> visitChildren() ------ [When reservedBytes() != 0] will acquire 'poolMutex_' again, **DEADLOCK!!**

So, addAggregateChild() indirectly calls visitChildren() [When reservedBytes() != 0], causing a deadlock.

To solve this problem, the easiest way is to simplify the error message printed when duplicate names appear.
After all, we only need to know the name of the parent pool, just like many other places in this class do.

[lingbin]

@zhztheplayer I created #10400 to fix this bug.