Is it redundant with two locks?

[onlyet]

Hey,bro
I have a question to ask for your help. I found your SafeQueue having a lock while ThreadPool also having a lock. Is they redundant？
I think, in your operator(),
If reserve SafeQueue，change the code to below:

void operator()()
{
	std::function<void()> func;
	bool dequeued;
	while (!m_pool->m_shutdown)
	{
		{
			if (m_pool->m_queue.empty())
			{
				std::unique_lock<std::mutex> lock(m_pool->m_conditional_mutex);
				m_pool->m_conditional_lock.wait(lock);
			}
			dequeued = m_pool->m_queue.dequeue(func);
		}
		if (dequeued)
		{
			func();
		}
	}
}

if use normal queue(without lock), operator() stay just same as your previous version
Please point out my mistake if i was wrong, thanks！

[onlyet]

the empty() and dequeue() are thread safe, so they needn't be put in lock of ThreadPool

[mtrebi]

My apologies for the late reply. I've moved to a different country and I no longer have much free time for personal projects and I do like to read questions in Github very carefully.

Regarding your question, I first implemented the SafeQueue as something separated from the ThreadPool. The SafeQueue can be used in some other projects without any modification since it's something already functional on its own.

Now, the answer. I do think the lock is necessary. The SafeQueue is safe (hehe) but the conditional_mutex and conditional_lock are there not to protect the queue but to be used as a signalers and notifiers so that they know when can sleep and when they should work.

If you replace the SafeQueue with a standard queue, How can you make sure that dequeued = m_pool->m_queue.dequeue(func); is not getting executed from two different places at the same time? There's no lock in the Thread Pool to protect you from that.

Hope this helps, and again, accept my apologies.

Mariano.