mrhhsg opened a new pull request, #69270:
URL: https://github.com/apache/doris/pull/69270
### What problem does this PR solve?
Issue Number: None
Related PR: #69208
Problem Summary: `PipelineTask::stop_if_finished()` is called by
`FragmentMgr::cancel_worker()` (through
`PipelineFragmentContext::clear_finished_tasks()`) on the background cancel
thread. It copied the `_sink` shared_ptr without any lock. At the same time a
scheduler worker can run `PipelineTask::finalize()` for the same task, which
resets `_sink` while holding `_dependency_lock`. Copying and resetting the same
shared_ptr instance from two threads without synchronization is a data race
(undefined behavior): the cancel thread can observe a torn pointer/control
block and then call `is_finished()` on an object that is being released. Every
other cross-thread reader of `_sink` (`debug_string()`, the hybrid scheduler
submit path) is already synchronized; `stop_if_finished()` was the only one
left.
Fix: take `_dependency_lock` in `stop_if_finished()`, return if the task is
already finalized, and copy `_sink` under the lock, following the pattern of
`debug_string()`. `is_finished()` and `terminate()` are called after the lock
is released, because `terminate()` acquires the same non-recursive mutex. A
debug point inside the critical section lets the unit test hold the lock
deterministically.
This is the branch-4.2 backport of #69208. branch-4.2 still has a single
`_dependency_lock` instead of the `_dependency_lifecycle_lock` used on master,
and `stop_if_finished()` calls `terminate()` instead of
`unblock_all_dependencies()`. Only those two branch-specific call sites are
adapted; the fix intent and the test are unchanged.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Added `PipelineTaskTest.TEST_STOP_IF_FINISHED_EXCLUDES_FINALIZE`,
adapted from the original PR: it holds `stop_if_finished()` inside the critical
section, checks that a concurrent `finalize()` is blocked and cannot reset
`_sink`, and verifies that a finalized task is skipped without touching the
cleared `_sink`.
- All `PipelineTaskTest` cases pass: 23/23.
- `clang-format --dry-run --Werror` passes on the two changed files.
- No regression test: the race window is a few instructions wide and
cannot be triggered reliably from SQL.
- Behavior changed: No
- Does this need documentation: No
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
