Copilot commented on code in PR #57462:
URL: https://github.com/apache/doris/pull/57462#discussion_r2472104874
##########
be/src/runtime_filter/runtime_filter_mgr.h:
##########
@@ -72,7 +72,7 @@ struct GlobalMergeContext {
std::vector<TRuntimeFilterTargetParamsV2> targetv2_info;
std::unordered_set<UniqueId> arrive_id;
std::vector<PNetworkAddress> source_addrs;
- bool done = false;
+ std::atomic<bool> done = false;
Review Comment:
The header file is missing the `#include <atomic>` directive needed for
`std::atomic<bool>`. While this may compile due to transitive includes, it
violates best practices of explicitly including headers for types directly used
in the file.
##########
be/src/runtime_filter/runtime_filter_mgr.cpp:
##########
@@ -356,6 +356,12 @@ Status
RuntimeFilterMergeControllerEntity::_send_rf_to_target(GlobalMergeContext
int
execution_timeout) {
DCHECK_GT(cnt_val.targetv2_info.size(), 0);
+ if (cnt_val.done) {
+ return Status::InternalError("Runtime filter has been sent",
+ cnt_val.merger->debug_string());
+ }
+ cnt_val.done = true;
Review Comment:
The check-then-set pattern on the atomic `done` flag is not thread-safe.
Multiple threads could pass the check at line 359 simultaneously before any
sets `done = true` at line 363. Use `compare_exchange_strong` or protect this
section with `cnt_val.mtx` to ensure atomicity of the entire check-and-set
operation.
```suggestion
{
std::lock_guard<std::mutex> l(cnt_val.mtx);
if (cnt_val.done) {
return Status::InternalError("Runtime filter has been sent",
cnt_val.merger->debug_string());
}
cnt_val.done = true;
}
```
--
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]
