Skip to content

Commit b94e4bb

Browse files
committed
[BugFix] Unschedule global RF timer in PipelineDriver destructor
A queued or blocked PipelineDriver that is abandoned when the driver executor is closed during shutdown is destroyed without going through finalize(), which was the only place the global runtime-filter timer (RFScanWaitTimeout) was unscheduled. Because the bthread TimerThread keeps only a raw pointer to the task, it can then fire on a task whose sole owning shared_ptr (PipelineDriver::_global_rf_timer) is already gone. PipelineTimerTask::doRun() calls shared_from_this() on that freed object and throws std::bad_weak_ptr, aborting the process: terminate called after throwing an instance of 'std::bad_weak_ptr' RunTimerTask -> bthread::TimerThread::run This is the abandoned-driver counterpart to the query-context-destruction race fixed in #73082, which only unscheduled from finalize(); the throwing shared_from_this() in doRun() was introduced in #72058. Move the unschedule into a helper called from both finalize() and ~PipelineDriver(). The helper moves the shared_ptr into a local so the task stays alive across unschedule_and_join(), closing the window where a concurrently running doRun() could observe a zero use_count. Add unit tests covering the destructor unscheduling a pending timer and the destructor blocking until an in-flight timer task finishes.
1 parent 202255e commit b94e4bb

3 files changed

Lines changed: 75 additions & 5 deletions

File tree

be/src/exec/runtime/pipeline_driver.cpp

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,10 @@ PipelineDriver::PipelineDriver()
102102
_driver_id(0) {}
103103

104104
PipelineDriver::~PipelineDriver() noexcept {
105+
// Queued or blocked drivers abandoned when the driver executor is closed during shutdown are
106+
// destroyed without going through finalize(), so unschedule the global runtime-filter timer here
107+
// as well; otherwise the timer thread may run a task whose owning shared_ptr is already gone.
108+
_unschedule_global_rf_timer();
105109
if (_workgroup != nullptr) {
106110
_workgroup->decr_num_running_drivers();
107111
}
@@ -884,11 +888,7 @@ void PipelineDriver::finalize(RuntimeState* runtime_state, DriverState state) {
884888

885889
_update_driver_level_timer();
886890

887-
if (_global_rf_timer != nullptr) {
888-
if (_pipeline_timer_context != nullptr) {
889-
_pipeline_timer_context->unschedule_and_join(_global_rf_timer.get());
890-
}
891-
}
891+
_unschedule_global_rf_timer();
892892

893893
if (_driver_observer != nullptr) {
894894
_driver_observer->on_driver_finished(runtime_state);
@@ -939,6 +939,16 @@ void PipelineDriver::_update_global_rf_timer() {
939939
WARN_IF_ERROR(_pipeline_timer_context->schedule(_global_rf_timer.get(), abstime), "schedule:");
940940
}
941941

942+
void PipelineDriver::_unschedule_global_rf_timer() noexcept {
943+
// Move the task out first so it stays alive while we synchronize with a callback that may already
944+
// be running on the timer thread but has not yet taken its shared_from_this() reference. Otherwise
945+
// doRun() could observe a zero use_count and throw std::bad_weak_ptr.
946+
auto task = std::move(_global_rf_timer);
947+
if (task != nullptr && _pipeline_timer_context != nullptr) {
948+
_pipeline_timer_context->unschedule_and_join(task.get());
949+
}
950+
}
951+
942952
std::string PipelineDriver::_build_readable_string(bool use_raw_name) const {
943953
std::stringstream ss;
944954
std::string block_reasons = "";

be/src/exec/runtime/pipeline_driver.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,7 @@ class PipelineDriver {
446446

447447
// used in event scheduler
448448
void _update_global_rf_timer();
449+
void _unschedule_global_rf_timer() noexcept;
449450

450451
// Helper function to build readable string with option to use raw operator names
451452
std::string _build_readable_string(bool use_raw_name) const;

be/test/exec/pipeline/schedule/pipeline_timer_test.cpp

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "butil/time.h"
2929
#include "common/brpc/brpc_stub_cache.h"
3030
#include "compute_env/pipeline/pipeline_timer_context.h"
31+
#include "exec/runtime/pipeline_driver.h"
3132
#include "exec_primitive/pipeline/primitives/pipeline_observer.h"
3233
#include "gtest/gtest.h"
3334
#include "runtime/runtime_state.h"
@@ -73,6 +74,18 @@ class LightProbe final : public LightTimerTask {
7374
std::atomic<bool> ran{false};
7475
};
7576

77+
// Reaches PipelineDriver's protected default constructor and its protected global-RF-timer members
78+
// so a test can drive the destructor cleanup path without standing up a full fragment.
79+
class TimerTestPipelineDriver final : public PipelineDriver {
80+
public:
81+
TimerTestPipelineDriver() = default;
82+
83+
void register_global_rf_timer(PipelineTimerContextPtr context, std::shared_ptr<PipelineTimerTask> task) {
84+
_pipeline_timer_context = std::move(context);
85+
_global_rf_timer = std::move(task);
86+
}
87+
};
88+
7689
class CountingObserver final : public PipelineObserver {
7790
public:
7891
void source_trigger() override {
@@ -303,6 +316,52 @@ TEST_F(PipelineTimerTaskTest, pipeline_timer_context_submits_batched_rf_timeout_
303316
context.clear_rf_timeout_tasks();
304317
}
305318

319+
// A queued or blocked driver abandoned when the driver executor is closed during shutdown is
320+
// destroyed without going through finalize(). The destructor must still unschedule the global
321+
// runtime-filter timer, otherwise the timer thread runs a task whose owning shared_ptr is gone and
322+
// doRun()'s shared_from_this() throws std::bad_weak_ptr.
323+
TEST_F(PipelineTimerTaskTest, pipeline_driver_destructor_unschedules_global_rf_timer) {
324+
auto context = std::make_shared<PipelineTimerContext>(&timer);
325+
auto task = std::make_shared<ProbeTask>();
326+
ASSERT_OK(context->schedule(task.get(), future_abstime(3600)));
327+
ASSERT_NE(0, task->tid());
328+
329+
auto driver = std::make_unique<TimerTestPipelineDriver>();
330+
driver->register_global_rf_timer(context, task);
331+
332+
// Intentionally skip finalize().
333+
driver.reset();
334+
335+
// The destructor must already have removed the task; a second unschedule finds nothing.
336+
EXPECT_EQ(-1, timer.unschedule(task.get()));
337+
}
338+
339+
// The destructor must block until an already-running global RF timer task returns, mirroring
340+
// finalize()'s unschedule_and_join, so the task is never freed while doRun() is still executing.
341+
TEST_F(PipelineTimerTaskTest, pipeline_driver_destructor_waits_for_running_global_rf_timer) {
342+
auto context = std::make_shared<PipelineTimerContext>(&timer);
343+
auto task = std::make_shared<ProbeTask>();
344+
task->block_until_released = true;
345+
ASSERT_OK(context->schedule(task.get(), past_abstime()));
346+
task->run_enter.acquire();
347+
348+
auto driver = std::make_unique<TimerTestPipelineDriver>();
349+
driver->register_global_rf_timer(context, task);
350+
351+
std::atomic<bool> destructor_returned{false};
352+
std::thread destructor([driver = std::move(driver), &destructor_returned]() mutable {
353+
driver.reset();
354+
destructor_returned.store(true, std::memory_order_release);
355+
});
356+
357+
std::this_thread::sleep_for(std::chrono::milliseconds(50));
358+
EXPECT_FALSE(destructor_returned.load(std::memory_order_acquire));
359+
360+
task->run_gate.release();
361+
destructor.join();
362+
EXPECT_TRUE(destructor_returned.load(std::memory_order_acquire));
363+
}
364+
306365
// LightTimerTask goes through a separate schedule/unschedule pair and lacks the
307366
// _finished / _has_consumer protocol. Cover it to prevent regressions in the
308367
// other overload of PipelineTimer.

0 commit comments

Comments
 (0)