Skip to content

[fix](exec) Fix data race on _queue_deps and _parents in ExchangeSinkBuffer#62040

Draft
Mryange wants to merge 1 commit intoapache:masterfrom
Mryange:ThreadSafetyAnalysis-demo
Draft

[fix](exec) Fix data race on _queue_deps and _parents in ExchangeSinkBuffer#62040
Mryange wants to merge 1 commit intoapache:masterfrom
Mryange:ThreadSafetyAnalysis-demo

Conversation

@Mryange
Copy link
Copy Markdown
Contributor

@Mryange Mryange commented Apr 2, 2026

What problem does this PR solve?

Problem Summary:

exchange_sink_buffer.cpp:569:24: error: reading variable '_queue_deps' requires holding mutex '_m'
[-Werror,-Wthread-safety-analysis]
  569 |         for (auto& dep : _queue_deps) {
      |                        ^

ExchangeSinkBuffer has two member fields _queue_deps and _parents that are protected by mutex _m. However, multiple code paths access these fields without holding _m, leading to data races.

Specifically, add_block(), _send_rpc(), and _set_receiver_eof() iterate _queue_deps while only holding the per-RpcInstance mutex (instance_data.mutex), and _turn_off_channel() iterates _parents also without holding _m. Concurrently, set_dependency() acquires _m and pushes to both _queue_deps and _parents. Since the two lock domains are disjoint, if a new sink registers via set_dependency() while another thread is iterating _queue_deps, the std::vector may be reallocated underneath the iterator, causing use-after-free or corrupted iteration. This is undefined behavior.

Beyond the data race on the vector itself, there is also a missed-wakeup problem: the check-then-act pattern on _total_queue_size (an std::atomic<int>) versus the _queue_deps block/unblock iteration is not atomic. Two threads holding different instance mutexes can interleave such that one thread blocks all dependencies right after another thread has already checked and decided to unblock, resulting in a permanently blocked pipeline (deadlock).

Approach

This PR introduces Clang's -Wthread-safety static analysis on exchange_sink_buffer.cpp to mechanically detect all unprotected accesses, then fixes all reported violations.

1. Thread Safety Annotation Infrastructure

Added be/src/common/thread_safety_annotations.h which provides:

  • TSA_GUARDED_BY, TSA_REQUIRES, TSA_ACQUIRE, TSA_RELEASE, etc. — standard Clang thread safety annotation macros
  • AnnotatedMutex — a std::mutex wrapper annotated with TSA_CAPABILITY("mutex")
  • AnnotatedLockGuard<MutexType> — an RAII lock guard annotated with TSA_SCOPED_CAPABILITY

These are no-ops when compiling with non-Clang compilers.

2. Annotations Applied to ExchangeSinkBuffer

In exchange_sink_buffer.h:

  • std::mutex _mAnnotatedMutex _m
  • _queue_deps marked with TSA_GUARDED_BY(_m)
  • _parents marked with TSA_GUARDED_BY(_m)
  • set_dependency() uses AnnotatedLockGuard instead of std::lock_guard

In be/src/exec/CMakeLists.txt:

  • Added set_source_files_properties(... PROPERTIES COMPILE_FLAGS "-Wthread-safety") for exchange_sink_buffer.cpp, so the analysis is enabled per-file without affecting the rest of the build.

3. Compilation Errors Detected

With -Wthread-safety enabled, Clang reported 8 errors at 4 locations:

Location Line Field Access Pattern
add_block() ~189 _queue_deps Iterates to call dep->block() — only holds instance_data.mutex, not _m
_send_rpc() ~389 _queue_deps Iterates to call dep->set_ready() — only holds instance_data.mutex, not _m
_set_receiver_eof() ~569 _queue_deps Iterates to call dep->set_ready() — holds no lock at all
_turn_off_channel() ~590 _parents Iterates to call parent->on_channel_finished() — holds instance_data.mutex, not _m

4. Fixes Applied

All four sites now acquire _m (via AnnotatedLockGuard l(_m)) before iterating the guarded fields:

  • add_block(): Wrapped the _total_queue_size > _queue_capacity check and _queue_deps iteration in AnnotatedLockGuard l(_m).
  • _send_rpc(): Wrapped the _total_queue_size <= _queue_capacity check and _queue_deps iteration in AnnotatedLockGuard l(_m).
  • _set_receiver_eof(): Wrapped the _total_queue_size <= _queue_capacity check and _queue_deps iteration in AnnotatedLockGuard l(_m).
  • _turn_off_channel(): Wrapped the _parents iteration in AnnotatedLockGuard l(_m).

Lock ordering is always instance_data.mutex_m (the inner lock), so there is no deadlock risk.

After applying the fixes, the file compiles cleanly with zero thread-safety violations.

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@Thearas
Copy link
Copy Markdown
Contributor

Thearas commented Apr 2, 2026

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

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?

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 2, 2026

run buildall

@doris-robot
Copy link
Copy Markdown

TPC-H: Total hot run time: 29318 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 707d0b5adc069363393294f4e124e6fe7bc01175, data reload: false

------ Round 1 ----------------------------------
orders	Doris	NULL	NULL	0	0	0	NULL	0	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	17717	3745	3719	3719
q2	q3	10654	887	627	627
q4	4690	465	372	372
q5	7678	1336	1153	1153
q6	268	170	146	146
q7	950	957	772	772
q8	10352	1503	1347	1347
q9	6838	5378	5315	5315
q10	6334	2033	1768	1768
q11	467	279	291	279
q12	861	698	520	520
q13	18030	2800	2179	2179
q14	287	283	261	261
q15	q16	902	866	784	784
q17	1103	1123	732	732
q18	6447	5693	5654	5654
q19	1206	1310	1023	1023
q20	623	545	440	440
q21	4591	2317	1924	1924
q22	436	368	303	303
Total cold run time: 100434 ms
Total hot run time: 29318 ms

----- Round 2, with runtime_filter_mode=off -----
orders	Doris	NULL	NULL	150000000	42	6422171781	NULL	22778155	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	4054	3947	3962	3947
q2	q3	4579	4736	4101	4101
q4	2004	2068	1349	1349
q5	4910	4931	5226	4931
q6	196	165	131	131
q7	1954	1747	1599	1599
q8	3272	3003	3059	3003
q9	8079	8110	8041	8041
q10	4456	4427	4280	4280
q11	567	401	374	374
q12	666	708	484	484
q13	2453	2867	2167	2167
q14	286	289	259	259
q15	q16	743	765	679	679
q17	1225	1204	1151	1151
q18	7801	6885	6967	6885
q19	1075	1060	1090	1060
q20	2201	2197	1901	1901
q21	5905	5302	4761	4761
q22	538	473	402	402
Total cold run time: 56964 ms
Total hot run time: 51505 ms

@doris-robot
Copy link
Copy Markdown

TPC-DS: Total hot run time: 178990 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 707d0b5adc069363393294f4e124e6fe7bc01175, data reload: false

query5	3714	671	519	519
query6	356	234	205	205
query7	4214	576	340	340
query8	336	258	222	222
query9	8689	3998	3987	3987
query10	489	405	345	345
query11	6589	5479	5099	5099
query12	183	134	127	127
query13	1295	612	450	450
query14	5660	5189	4831	4831
query14_1	4199	4178	4141	4141
query15	213	204	186	186
query16	883	466	490	466
query17	973	786	675	675
query18	2421	517	413	413
query19	252	221	190	190
query20	133	133	130	130
query21	197	142	115	115
query22	13614	13555	13385	13385
query23	17944	16910	16511	16511
query23_1	16588	16737	16567	16567
query24	7384	1747	1361	1361
query24_1	1336	1371	1352	1352
query25	527	510	447	447
query26	945	325	179	179
query27	2679	638	378	378
query28	4423	1896	1867	1867
query29	916	655	565	565
query30	298	248	201	201
query31	1097	1049	959	959
query32	86	77	77	77
query33	456	351	291	291
query34	1214	1117	655	655
query35	740	800	678	678
query36	1210	1211	1058	1058
query37	152	102	97	97
query38	3090	3038	2988	2988
query39	918	892	881	881
query39_1	837	829	823	823
query40	231	171	156	156
query41	66	63	64	63
query42	282	273	283	273
query43	311	329	281	281
query44	
query45	216	199	190	190
query46	1156	1208	816	816
query47	2330	2373	2216	2216
query48	405	425	304	304
query49	663	542	439	439
query50	723	288	221	221
query51	4367	4287	4271	4271
query52	281	282	276	276
query53	328	359	276	276
query54	336	296	273	273
query55	103	100	92	92
query56	342	337	329	329
query57	1679	1667	1572	1572
query58	299	277	279	277
query59	2901	2982	2739	2739
query60	335	341	326	326
query61	158	148	175	148
query62	680	622	557	557
query63	317	290	277	277
query64	4571	1438	1080	1080
query65	
query66	1335	517	381	381
query67	24553	24297	24183	24183
query68	
query69	456	351	317	317
query70	1027	990	1018	990
query71	367	323	309	309
query72	2932	2743	2445	2445
query73	795	756	443	443
query74	9842	9836	9582	9582
query75	3554	3360	3014	3014
query76	2389	1134	748	748
query77	404	413	351	351
query78	11304	11252	10735	10735
query79	1448	1095	822	822
query80	843	779	658	658
query81	451	284	238	238
query82	825	157	123	123
query83	369	294	258	258
query84	298	147	118	118
query85	841	526	444	444
query86	390	339	338	338
query87	3283	3236	3115	3115
query88	3521	2725	2673	2673
query89	476	413	378	378
query90	1619	180	170	170
query91	179	183	156	156
query92	96	83	82	82
query93	920	885	521	521
query94	560	367	312	312
query95	619	387	348	348
query96	1009	793	368	368
query97	2696	2696	2582	2582
query98	244	233	236	233
query99	1068	1090	983	983
Total cold run time: 247798 ms
Total hot run time: 178990 ms

@hello-stephen
Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 52.96% (20060/37879)
Line Coverage 36.54% (188283/515296)
Region Coverage 32.78% (146067/445656)
Branch Coverage 33.95% (63998/188494)

@hello-stephen
Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 92.31% (24/26) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 71.89% (26670/37098)
Line Coverage 54.84% (281731/513747)
Region Coverage 51.90% (233442/449769)
Branch Coverage 53.39% (100939/189060)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants