github-actions[bot] commented on code in PR #67755:
URL: https://github.com/apache/doris/pull/67755#discussion_r3975245658
##########
be/src/util/brpc_closure.h:
##########
@@ -118,9 +125,21 @@ class AutoReleaseClosure : public
google::protobuf::Closure {
~AutoReleaseClosure() override = default;
- // Will delete itself. all operations should be done in callback's call().
Run() only do one thing.
+ // Releases per-RPC resources, invokes the callback if it is still alive,
and then deletes itself.
void Run() override {
Defer defer {[&]() { delete this; }};
+
+ // The request attachment is no longer needed after brpc finishes the
RPC. It can contain a
+ // large serialized runtime filter, especially a Bloom filter. Since
the callback owner may
+ // retain cntl_ after this closure is deleted (and some callbacks may
also reuse it), keeping
+ // the attachment here would retain that memory until the next
Controller::Reset() or until
+ // the callback is destroyed.
+ //
+ // This must be cleared before call(): a callback may synchronously
start the next RPC and
+ // reuse the same Controller, in which case clearing it after call()
could erase the new
+ // RPC's request attachment.
+ cntl_->request_attachment().clear();
Review Comment:
[P1] Release attachments on pre-submit failures
This cleanup runs only after BRPC invokes `Run()`, but several current
callers can retain the same Controller without ever submitting the closure. For
example, `_send_rf_to_target()` stores the callback in
`cnt_val.publish_callbacks` and appends the potentially large shared filter
before stub lookup; either null-stub `continue` destroys the closure, yet the
vector-owned callback keeps `cntl_` and the whole attachment until stage
reset/query teardown. The HTTP exchange and tablet-writer paths likewise embed
a large block before fallible DNS/client setup while a Channel/writer member
still owns the callback. Please add explicit not-submitted cleanup or move the
fallible setup ahead of attachment construction; the new `Run()` cleanup cannot
cover these exits.
##########
be/src/exec/exchange/vdata_stream_sender.h:
##########
@@ -163,12 +163,9 @@ class Channel {
std::shared_ptr<ExchangeSendCallback<PTransmitDataResult>>
get_send_callback(RpcInstance* ins,
bool eos) {
- // here we reuse the callback because it's re-construction may be
expensive due to many parameters' capture
- if (!_send_callback) {
- _send_callback =
ExchangeSendCallback<PTransmitDataResult>::create_shared();
- } else {
- _send_callback->cntl_->Reset();
- }
+ // Keep the latest callback alive because AutoReleaseClosure
intentionally holds only a
+ // weak_ptr. See ExchangeSinkBuffer::_send_rpc() for the callback
replacement sequence.
+ _send_callback =
ExchangeSendCallback<PTransmitDataResult>::create_shared();
Review Comment:
[P2] Add a deterministic oracle for the callback lifetime fix
No changed or existing test exercises the unsafe sequence this line fixes.
`test_queue_size` queues multiple packets on one Channel but completes the
first with EOF, so the instance is turned off before RPC B is submitted;
`test_normal_end` reenters only onto different Channel objects, whose callbacks
were already distinct on the base. Please add a one-Channel, two-packet success
case that retains A's Controller/response, runs A so B is submitted while A is
still on the stack, and then proves B owns distinct state (plus the broadcast
counterpart). The attachment change also needs an oracle for clearing before
callback logic and for the never-submitted cleanup path.
--
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]
