westonpace commented on a change in pull request #10401:
URL: https://github.com/apache/arrow/pull/10401#discussion_r648541270
##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -452,6 +454,42 @@ TEST_F(TestThreadPool, QuickShutdown) {
add_tester.CheckNotAllComputed();
}
+TEST_F(TestThreadPool, DestroyWithoutShutdown) {
+ std::mutex mx;
+ std::condition_variable cv;
+ std::weak_ptr<ThreadPool> weak_pool;
+ bool ready = false;
+ bool completed = false;
+ {
+ auto pool = this->MakeThreadPool(1);
+ weak_pool = pool;
+ // Simulate Windows
+ pool->shutdown_on_destroy_ = false;
+
+ ASSERT_OK(pool->Spawn([&mx, &cv, &completed, &ready] {
+ std::unique_lock<std::mutex> lock(mx);
+ ready = true;
+ cv.notify_one();
Review comment:
I'm pretty sure you're right, notify may wake up more than one thread.
However, I don't think it can wake up a thread before it starts waiting. In
other words, a thread can never notify itself.
I can probably rewrite this with two conditions though to make it more
legible and guard against spurious wakeups. My goal here is to ensure that the
worker thread is spawned before I lose the reference to the thread pool.
Although, thinking on this again, the synchronization may not be needed at all
since the lambda that creates a copy of the thread pool's shared_ptr is
guaranteed to have been created before `Spawn` returns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
