Copilot commented on code in PR #61782:
URL: https://github.com/apache/doris/pull/61782#discussion_r3055260347
##########
be/src/exec/operator/exchange_sink_buffer.cpp:
##########
@@ -347,6 +347,7 @@ Status ExchangeSinkBuffer::_send_rpc(RpcInstance&
instance_data) {
}
// The eos here only indicates that the current exchange sink has
reached eos.
// However, the queue still contains data from other exchange
sinks, so RPCs need to continue being sent.
+ // `_send_rpc` must be the LAST operation in this function,
because it may reuse the callback!
Review Comment:
The comment says "`_send_rpc` must be the LAST operation in this function",
but there is still logic immediately after the call (the `if (!s)` block). This
is misleading for future maintainers; consider rewording to clarify what must
not happen after `_send_rpc` (e.g., no further callback reuse-sensitive
operations), or move the check into `_send_rpc` if that’s the intent.
```suggestion
// After `_send_rpc`, do not perform any further callback
reuse-sensitive work here,
// because `_send_rpc` may reuse the callback. Only handle its
returned `Status`.
```
##########
be/src/exec/runtime_filter/runtime_filter_producer.cpp:
##########
@@ -208,14 +161,16 @@ Status RuntimeFilterProducer::send_size(RuntimeState*
state, uint64_t local_filt
auto request = std::make_shared<PSendFilterSizeRequest>();
request->set_stage(_stage);
-
- auto callback =
DummyBrpcCallback<PSendFilterSizeResponse>::create_shared();
+ auto callback = SyncSizeCallback::create_shared(_dependency, _wrapper,
+
state->get_query_ctx()->weak_from_this());
+ // Store callback in the producer to keep it alive until the RPC completes.
+ // AutoReleaseClosure holds callbacks via weak_ptr, so without this the
callback
+ // would be destroyed when this function returns and error-path sub()
would never fire.
+ _sync_size_callback = callback;
Review Comment:
_sync_size_callback is used to keep the callback alive until the RPC
completes (because AutoReleaseClosure holds only a weak_ptr). However it is
never cleared after the RPC finishes, so the producer will retain the callback
(and its Dependency shared_ptr) for the rest of the producer lifetime. Consider
clearing/releasing _sync_size_callback in the callback completion path (both
success and error) to avoid unnecessary retention.
--
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]
