You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "mrhhsg (via GitHub)" <gi...@apache.org> on 2026/10/16 14:46:03 UTC
[PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
mrhhsg opened a new pull request, #69208:
URL: https://github.com/apache/doris/pull/69208
### What problem does this PR solve?
Issue Number: None
Related PR: None
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_lifecycle_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_lifecycle_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 `unblock_all_dependencies()` are called after the lock is released, because `unblock_all_dependencies()` acquires the same non-recursive mutex. A debug point inside the critical section lets the unit test hold the lock deterministically.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Added `PipelineTaskTest.TEST_STOP_IF_FINISHED_EXCLUDES_FINALIZE`: it holds `stop_if_finished()` inside the critical section and checks that a concurrent `finalize()` is blocked and cannot reset `_sink`, and that a finalized task is skipped. It fails without the lock and passes with it (200 repeated runs). All `PipelineTaskTest` cases pass.
- 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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6218909728
<details>
<summary>TPC-H: <b>Total hot run time: 29198 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit a0d1db785448979df4c64033a0b284d286d85da2, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17568 5676 5533 5533
q2 2099 335 243 243
q3 10236 1388 824 824
q4 4678 471 347 347
q5 7440 790 531 531
q6 192 193 160 160
q7 775 797 609 609
q8 9261 1358 1124 1124
q9 5560 4410 4246 4246
q10 6833 1298 1030 1030
q11 429 267 239 239
q12 630 411 284 284
q13 18090 3605 2972 2972
q14 275 267 245 245
q15 q16 742 725 685 685
q17 1200 741 590 590
q18 7037 6209 6178 6178
q19 1109 957 593 593
q20 392 331 235 235
q21 4827 2253 2381 2253
q22 377 318 277 277
Total cold run time: 99750 ms
Total hot run time: 29198 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6134 6120 6106 6106
q2 684 586 545 545
q3 5209 5375 4969 4969
q4 1990 2139 1536 1536
q5 5272 5188 5914 5188
q6 252 206 152 152
q7 2284 2021 1860 1860
q8 3141 2673 2727 2673
q9 8063 8041 8179 8041
q10 3757 3697 3320 3320
q11 640 425 400 400
q12 676 732 508 508
q13 3643 3934 3167 3167
q14 330 337 288 288
q15 q16 708 741 656 656
q17 8276 7680 6893 6893
q18 13147 12244 13029 12244
q19 925 827 806 806
q20 2180 2169 1906 1906
q21 5745 4693 4786 4693
q22 497 442 385 385
Total cold run time: 73553 ms
Total hot run time: 66336 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6218027042
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://doris.apache.org/community/how-to-contribute/review-and-ci).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6221624396
# BE Regression && UT Coverage Report
Increment line coverage `100% (0/0)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69208_a0d1db785448979df4c64033a0b284d286d85da2_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69208_a0d1db785448979df4c64033a0b284d286d85da2_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 77.19% (35749/46313) |
| Line Coverage | 62.43% (403564/646445) |
| Region Coverage | 58.93% (341059/578707) |
| Branch Coverage | 59.85% (157497/263136) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6219179849
<details>
<summary>ClickBench: <b>Total hot run time: 24.47 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit a0d1db785448979df4c64033a0b284d286d85da2, data reload: false
query1 0.01 0.00 0.01
query2 0.11 0.05 0.04
query3 0.27 0.11 0.11
query4 1.61 0.12 0.13
query5 0.24 0.24 0.25
query6 1.17 0.69 0.69
query7 0.03 0.01 0.01
query8 0.05 0.04 0.05
query9 0.40 0.34 0.36
query10 0.57 0.59 0.57
query11 0.20 0.15 0.14
query12 0.18 0.14 0.15
query13 0.49 0.51 0.49
query14 1.03 1.03 1.02
query15 0.63 0.60 0.62
query16 0.35 0.35 0.35
query17 1.12 1.17 1.13
query18 0.22 0.21 0.21
query19 2.11 1.95 1.95
query20 0.02 0.02 0.01
query21 15.50 0.27 0.15
query22 4.50 0.06 0.05
query23 16.26 0.31 0.11
query24 2.84 0.72 0.36
query25 0.12 0.06 0.04
query26 1.55 0.23 0.16
query27 0.06 0.07 0.06
query28 2.03 0.61 0.38
query29 12.48 4.21 3.30
query30 0.28 0.15 0.13
query31 2.77 0.55 0.33
query32 3.22 0.60 0.51
query33 3.11 3.25 3.20
query34 15.55 3.97 3.38
query35 3.35 3.35 3.33
query36 0.56 0.43 0.44
query37 0.10 0.06 0.06
query38 0.06 0.04 0.03
query39 0.04 0.03 0.03
query40 0.17 0.15 0.15
query41 0.08 0.03 0.02
query42 0.04 0.03 0.03
query43 0.04 0.04 0.03
Total cold run time: 95.52 s
Total hot run time: 24.47 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6218221023
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6219089300
<details>
<summary>TPC-DS: <b>Total hot run time: 152438 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit a0d1db785448979df4c64033a0b284d286d85da2, data reload: false
query5 4299 597 463 463
query6 423 212 194 194
query7 4803 429 222 222
query8 314 174 158 158
query9 8768 3960 3917 3917
query10 487 303 251 251
query11 6010 3773 3552 3552
query12 157 93 86 86
query13 1249 435 318 318
query14 6105 4052 3718 3718
query14_1 3504 3508 3550 3508
query15 203 201 181 181
query16 961 448 440 440
query17 865 664 544 544
query18 2418 430 313 313
query19 199 165 138 138
query20 86 85 85 85
query21 214 133 112 112
query22 12986 12940 12743 12743
query23 13501 13008 12651 12651
query23_1 12802 12704 12617 12617
query24 7124 827 508 508
query24_1 461 473 472 472
query25 504 387 344 344
query26 1266 255 137 137
query27 2766 431 267 267
query28 4657 1929 1942 1929
query29 1547 573 420 420
query30 293 221 182 182
query31 1012 917 795 795
query32 146 87 94 87
query33 493 287 233 233
query34 937 853 490 490
query35 737 749 651 651
query36 820 776 728 728
query37 124 96 88 88
query38 1804 1776 1697 1697
query39 785 808 752 752
query39_1 738 725 737 725
query40 219 112 93 93
query41 66 63 64 63
query42 92 83 80 80
query43 357 376 331 331
query44 1292 694 687 687
query45 190 174 167 167
query46 837 972 587 587
query47 2865 3019 2990 2990
query48 306 305 223 223
query49 566 396 277 277
query50 673 277 215 215
query51 10638 10522 10523 10522
query52 78 80 70 70
query53 179 207 149 149
query54 237 186 182 182
query55 73 70 62 62
query56 240 228 211 211
query57 1632 1583 1579 1579
query58 283 264 250 250
query59 2126 2166 1962 1962
query60 285 242 227 227
query61 149 147 142 142
query62 390 341 283 283
query63 197 163 163 163
query64 2767 959 800 800
query65 3317 3258 3237 3237
query66 1782 426 311 311
query67 20260 20065 19900 19900
query68 2935 976 609 609
query69 386 296 257 257
query70 862 794 786 786
query71 291 225 214 214
query72 2827 2690 2350 2350
query73 526 532 312 312
query74 4590 4469 4339 4339
query75 2322 2292 1927 1927
query76 2316 1025 603 603
query77 340 385 286 286
query78 9709 9765 9169 9169
query79 1018 859 521 521
query80 895 414 340 340
query81 609 318 266 266
query82 583 146 110 110
query83 294 200 175 175
query84 316 117 101 101
query85 1007 409 361 361
query86 382 233 228 228
query87 2005 1957 1810 1810
query88 3636 2714 2710 2710
query89 342 295 255 255
query90 1743 185 183 183
query91 160 150 123 123
query92 95 90 92 90
query93 1031 992 568 568
query94 522 305 291 291
query95 580 393 317 317
query96 671 518 238 238
query97 2446 2423 2333 2333
query98 161 160 151 151
query99 769 754 629 629
Total cold run time: 234831 ms
Total hot run time: 152438 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg commented on PR #69208:
URL: https://github.com/apache/doris/pull/69208#issuecomment-6218047037
/review-light
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Copy PipelineTask::_sink under the lifecycle lock in stop_if_finished [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg merged PR #69208:
URL: https://github.com/apache/doris/pull/69208
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org