diff --git a/src/actions/test_node.cpp b/src/actions/test_node.cpp index 6990ffe14..c00c989bc 100644 --- a/src/actions/test_node.cpp +++ b/src/actions/test_node.cpp @@ -84,8 +84,10 @@ struct TestNode::PImpl ScriptFunction success_executor; ScriptFunction failure_executor; ScriptFunction post_executor; - TimerQueue<> timer; std::atomic_bool completed = false; + // Keep last: ~TimerQueue() joins the worker thread, so this must be destroyed + // before completed, which the timer handler touches. + TimerQueue<> timer; }; TestNode::TestNode(const std::string& name, const NodeConfig& config, @@ -133,17 +135,20 @@ NodeStatus TestNode::onStart() // convert this in an asynchronous operation. Use another thread to count // a certain amount of time. _p->completed = false; - _p->timer.add(std::chrono::milliseconds(_p->config->async_delay), [this](bool aborted) { - if(!aborted) - { - _p->completed.store(true); - this->emitWakeUpSignal(); - } - else - { - _p->completed.store(false); - } - }); + // The handler must not read _p: ~TimerQueue() runs it (aborted) while the + // PImpl is being deleted, and std::unique_ptr may already hold nullptr then. + _p->timer.add(std::chrono::milliseconds(_p->config->async_delay), + [this, p = _p.get()](bool aborted) { + if(!aborted) + { + p->completed.store(true); + this->emitWakeUpSignal(); + } + else + { + p->completed.store(false); + } + }); return NodeStatus::RUNNING; } diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp index 1883b0385..9a0e84bea 100644 --- a/tests/gtest_substitution.cpp +++ b/tests/gtest_substitution.cpp @@ -589,3 +589,18 @@ TEST(Substitution, IncompatibleTypeSubstitutionThrows) EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS); } } + +// Destroying a TestNode while its async_delay timer is pending makes +// ~TimerQueue() run the handler (aborted) during the PImpl deletion. The +// handler used to reach the PImpl through _p, which std::unique_ptr may have +// already reset to nullptr at that point (libc++ does). +TEST(Substitution, DestroyTestNodeWithPendingAsyncDelay) +{ + auto test_config = std::make_shared(); + test_config->return_status = NodeStatus::SUCCESS; + test_config->async_delay = std::chrono::milliseconds(10000); + + auto node = std::make_unique("mock", NodeConfig{}, test_config); + ASSERT_EQ(node->executeTick(), NodeStatus::RUNNING); + node.reset(); +}