Storages: use-after-free race between SegmentReadTaskScheduler and SegmentReaderPoolManager shutdown#10917
Conversation
…ReaderPoolManager shutdown (pingcap#10903) The schedLoop background thread could still be running when SegmentReaderPoolManager::stop() destroyed all reader_pools and their WorkQueue mutexes. Subsequent scheduleOneRound() calls would then access already-destroyed objects, causing SIGSEGV. Fix by adding a public stop() method to SegmentReadTaskScheduler and calling it before SegmentReaderPoolManager::stop() in both production (Server.cpp) and test (gtests_dbms_main.cpp) shutdown paths. Also fix: - rename std::atomic<bool> stop -> stop_flag to avoid name collision - fix typo: reapPeningPools -> reapPendingPools in header comment - fix typo: max_free_threds -> max_free_threads in test main
ti-chi-bot
Bot
added
release-note-none
📝 WalkthroughWalkthroughAdds a public ChangesSegmentReadTaskScheduler shutdown ordering fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp (1)
37-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop re-queued merged tasks after scheduler stop.
stop()joins onlysched_thread; the next shutdown step stopsSegmentReaderPoolManager, and exiting readers can still callpushMergedTask()for unfinished work. Since no scheduler remains to drainmerged_task_pool, guardpushMergedTask()on the stop flag so shutdown releases the task instead of retaining read resources until singleton teardown.Suggested guard
- void pushMergedTask(const MergedTaskPtr & p) { merged_task_pool.push(p); } + void pushMergedTask(const MergedTaskPtr & p) + { + if (isStop()) + return; + merged_task_pool.push(p); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp` around lines 37 - 43, The issue is that after stop() joins the scheduler thread, the SegmentReaderPoolManager shutdown continues and exiting readers can still invoke pushMergedTask(), which adds tasks to merged_task_pool that will never be drained since the scheduler is no longer running. Guard the pushMergedTask() method to check the stop flag before accepting new tasks; if the scheduler has already been stopped, the method should drop or reject the task instead of queuing it to prevent resources from being retained until singleton teardown.
🧹 Nitpick comments (1)
dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.h (1)
104-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
stop_flagtostopFlag.This new member uses snake_case while the project guideline requires camelCase variables; update the declaration and the
setStop()/isStop()references together.Suggested rename
- std::atomic<bool> stop_flag{false}; + std::atomic<bool> stopFlag{false};- stop_flag.store(true, std::memory_order_relaxed); + stopFlag.store(true, std::memory_order_relaxed); - return stop_flag.load(std::memory_order_relaxed); + return stopFlag.load(std::memory_order_relaxed);As per coding guidelines,
Method and variable names should use camelCase (e.g., readBlock, totalBytes).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.h` at line 104, The member variable `stop_flag` in the SegmentReadTaskScheduler class uses snake_case naming which violates the project's camelCase guideline for variables. Rename `stop_flag` to `stopFlag` in the member declaration and update all references to this variable throughout the class, including in the setStop() and isStop() methods, to comply with the coding standards.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp`:
- Around line 37-43: The issue is that after stop() joins the scheduler thread,
the SegmentReaderPoolManager shutdown continues and exiting readers can still
invoke pushMergedTask(), which adds tasks to merged_task_pool that will never be
drained since the scheduler is no longer running. Guard the pushMergedTask()
method to check the stop flag before accepting new tasks; if the scheduler has
already been stopped, the method should drop or reject the task instead of
queuing it to prevent resources from being retained until singleton teardown.
---
Nitpick comments:
In `@dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.h`:
- Line 104: The member variable `stop_flag` in the SegmentReadTaskScheduler
class uses snake_case naming which violates the project's camelCase guideline
for variables. Rename `stop_flag` to `stopFlag` in the member declaration and
update all references to this variable throughout the class, including in the
setStop() and isStop() methods, to comply with the coding standards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1e75f63-fdc9-4953-b79d-7c2e51022c84
📒 Files selected for processing (4)
dbms/src/Server/Server.cppdbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cppdbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.hdbms/src/TestUtils/gtests_dbms_main.cpp
|
/retest |
SegmentReader threads may call pushMergedTask() for unfinished work after stop() has joined schedLoop. Since no scheduler remains to drain merged_task_pool, guard the method with isStop() to discard tasks instead of leaking them until singleton teardown.
|
/retest |
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
/test pull-unit-next-gen |
|
/test pull-integration-test |
|
/retest |
JaySon-Huang
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JinheLin, yongman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
/test pull-integration-test |
1 similar comment
|
/test pull-integration-test |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/cherry-pick release-nextgen-202603 |
|
@JaySon-Huang: once the present PR merges, I will cherry-pick it on top of release-nextgen-202603 in the new PR and assign it to you. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@JaySon-Huang: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
…gmentReaderPoolManager shutdown (#10917) (#10922) close #10903\n\nAdd a public `stop()` method to `SegmentReadTaskScheduler` that sets the stop flag and joins the background thread. Call it **before** `SegmentReaderPoolManager::stop()` in both production (`Server.cpp`) and test (`gtests_dbms_main.cpp`) shutdown paths. - `SegmentReadTaskScheduler.h`: expose `stop()` as public method; rename `std::atomic<bool> stop` → `stop_flag` to avoid name collision - `SegmentReadTaskScheduler.cpp`: implement `stop()`, destructor delegates to it - `Server.cpp`: call `SegmentReadTaskScheduler::instance().stop()` before `SegmentReaderPoolManager::instance().stop()` - `gtests_dbms_main.cpp`: same reorder Also fix two minor typos along the way: - `reapPeningPools` → `reapPendingPools` in class comment - `max_free_threds` → `max_free_threads` in test main\n\nCo-authored-by: JaySon-Huang <tshent@qq.com>
…gmentReaderPoolManager shutdown (pingcap#10917) close pingcap#10903\n\nAdd a public `stop()` method to `SegmentReadTaskScheduler` that sets the stop flag and joins the background thread. Call it **before** `SegmentReaderPoolManager::stop()` in both production (`Server.cpp`) and test (`gtests_dbms_main.cpp`) shutdown paths. - `SegmentReadTaskScheduler.h`: expose `stop()` as public method; rename `std::atomic<bool> stop` → `stop_flag` to avoid name collision - `SegmentReadTaskScheduler.cpp`: implement `stop()`, destructor delegates to it - `Server.cpp`: call `SegmentReadTaskScheduler::instance().stop()` before `SegmentReaderPoolManager::instance().stop()` - `gtests_dbms_main.cpp`: same reorder Also fix two minor typos along the way: - `reapPeningPools` → `reapPendingPools` in class comment - `max_free_threds` → `max_free_threads` in test main
4 participants
What problem does this PR solve?
Issue Number: close #10903
Problem Summary:
During shutdown,
SegmentReaderPoolManager::stop()destroys allreader_poolsand theirWorkQueuemutexes, butSegmentReadTaskScheduler'sschedLoopbackground thread is still running. WhenschedLoopwakes from its 2ms sleep, it callsscheduleOneRound()→SegmentReaderPoolManager::addTask()→ accesses already-destroyedreader_pools, causing SIGSEGV (use-after-free).What is changed and how it works?
Check List
Tests
Manual test:
./gtests_dbms --gtest_filter="DeltaMergeStoreTest.ReadLegacyStringDataCFTiny"— test passes and process exits cleanly without SIGSEGV.Side effects
Documentation
Release note
Summary by CodeRabbit
Release Notes