github-actions[bot] commented on code in PR #61782:
URL: https://github.com/apache/doris/pull/61782#discussion_r3055431950
##########
be/src/exec/runtime_filter/runtime_filter.cpp:
##########
@@ -26,6 +28,34 @@
namespace doris {
#include "common/compile_check_begin.h"
+
+namespace {
+template <typename Response, typename Lock>
+class ManagedTrivialBrpcCallback final : public TrivialBrpcCallback<Response> {
+ ENABLE_FACTORY_CREATOR(ManagedTrivialBrpcCallback);
+
+public:
+ ManagedTrivialBrpcCallback(Lock* lock, std::vector<std::shared_ptr<void>>*
callbacks,
+ std::weak_ptr<QueryContext> context = {})
+ : TrivialBrpcCallback<Response>(std::move(context)),
+ _lock(lock),
+ _callbacks(callbacks) {}
+
+ void call() override {
+ TrivialBrpcCallback<Response>::call();
Review Comment:
`TrivialBrpcCallback<Response>::call()` can cancel the query before this
helper reacquires `_lock`. In the runtime-filter RPC paths that means query
teardown can destroy the owning `RuntimeFilter` while this callback is still on
the stack, so the subsequent `lock_guard` + `_callbacks->erase(...)`
dereferences freed memory. The new callback-retention vector only keeps the
callback alive; it does not keep the owner alive. Please move this bookkeeping
into teardown-safe shared state, or drain/join these callbacks before owner
destruction.
##########
be/src/exec/runtime_filter/runtime_filter_mgr.cpp:
##########
@@ -40,13 +41,42 @@
#include "runtime/exec_env.h"
#include "runtime/memory/mem_tracker.h"
#include "runtime/query_context.h"
+#include "runtime/runtime_profile.h"
#include "runtime/runtime_state.h"
#include "runtime/thread_context.h"
#include "util/brpc_client_cache.h"
#include "util/brpc_closure.h"
namespace doris {
#include "common/compile_check_begin.h"
+
+namespace {
+template <typename Response, typename Lock>
+class ManagedTrivialBrpcCallback final : public TrivialBrpcCallback<Response> {
+ ENABLE_FACTORY_CREATOR(ManagedTrivialBrpcCallback);
+
+public:
+ ManagedTrivialBrpcCallback(Lock* lock, std::vector<std::shared_ptr<void>>*
callbacks,
+ std::weak_ptr<QueryContext> context = {})
+ : TrivialBrpcCallback<Response>(std::move(context)),
+ _lock(lock),
+ _callbacks(callbacks) {}
+
+ void call() override {
+ TrivialBrpcCallback<Response>::call();
Review Comment:
Same lifecycle problem here: `TrivialBrpcCallback<Response>::call()` may
cancel the query, and that can tear down `RuntimeFilterMergeControllerEntity`
before this helper reaches `_callbacks->erase(...)`. Because `_lock` and
`_callbacks` are raw back-pointers into the owner, an error RPC can now turn
into a use-after-free during review cleanup. This needs a teardown barrier or
owner-independent callback state.
--
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]
