Copilot commented on code in PR #61768:
URL: https://github.com/apache/doris/pull/61768#discussion_r2993182618
##########
be/src/exec/pipeline/pipeline_task.cpp:
##########
@@ -504,7 +504,6 @@ Status PipelineTask::execute(bool* done) {
// if Thread A observes B's effect (_always_ready=true), it is
guaranteed to also observe
// A's effect (_wake_up_early=true) on this second read, ensuring
terminate() is called.
if (_wake_up_early) {
- terminate();
THROW_IF_ERROR(_root->terminate(_state));
THROW_IF_ERROR(_sink->terminate(_state));
}
Review Comment:
The inline comment block still says the ordering “ensures terminate() is
called”, but this branch no longer calls `PipelineTask::terminate()` (only
`_root->terminate()` / `_sink->terminate()`). Please update the comment to
reflect the new behavior (i.e., what is being guaranteed now is that operator
termination runs, while dependency unblocking via `PipelineTask::terminate()`
happens elsewhere).
##########
be/src/exec/pipeline/pipeline.cpp:
##########
@@ -125,10 +125,6 @@ void Pipeline::make_all_runnable(PipelineId wake_by) {
for (auto* task : _tasks) {
if (task) {
task->set_wake_up_early(wake_by);
- }
- }
- for (auto* task : _tasks) {
- if (task) {
task->terminate();
}
Review Comment:
`make_all_runnable()` now calls `terminate()` immediately after
`set_wake_up_early()` for each task. This changes the ordering from “set
wake_up_early for all tasks, then terminate all tasks” to an interleaved
per-task ordering. If any `Dependency` objects are shared across tasks (e.g.
local-exchange dependencies can be shared), terminating an earlier task can set
`_always_ready=true` on a shared dependency before later tasks have
`_wake_up_early=true`, reintroducing the exact race described in
`PipelineTask::execute()` (task can observe `_always_ready` and close without
running operator terminate). Consider restoring the two-phase loop (set
wake_up_early for all tasks first, then terminate all tasks) or otherwise
ensuring all tasks have `_wake_up_early` set before any task mutates shared
dependencies via `terminate()`.
--
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]
