github-actions[bot] commented on code in PR #61782:
URL: https://github.com/apache/doris/pull/61782#discussion_r3055247741
##########
be/src/util/brpc_closure.h:
##########
@@ -56,56 +76,38 @@ class DummyBrpcCallback {
// So that we need keep a shared ptr here to ensure that brpc could use
req/rep
// at any stage.
std::shared_ptr<Response> response_;
+
+private:
+ std::weak_ptr<QueryContext> _context;
};
// The closure will be deleted after callback.
// It could only be created by using shared ptr or unique ptr.
-// It will hold a weak ptr of T and call run of T
-// Callback() {
-// xxxx;
-// public
-// void run() {
-// logxxx
-// }
-// }
-//
-// std::shared_ptr<Callback> b;
-//
+// Example:
// std::unique_ptr<AutoReleaseClosure> a(b);
// brpc_call(a.release());
-template <typename T>
-concept HasStatus = requires(T* response) { response->status(); };
-
template <typename Request, typename Callback>
class AutoReleaseClosure : public google::protobuf::Closure {
using Weak = typename std::shared_ptr<Callback>::weak_type;
using ResponseType = typename Callback::ResponseType;
ENABLE_FACTORY_CREATOR(AutoReleaseClosure);
public:
- AutoReleaseClosure(std::shared_ptr<Request> req, std::shared_ptr<Callback>
callback,
- std::weak_ptr<QueryContext> context = {},
std::string_view error_msg = {})
- : request_(req), callback_(callback), context_(std::move(context))
{
+ AutoReleaseClosure(std::shared_ptr<Request> req, std::shared_ptr<Callback>
callback)
+ : request_(req), callback_(callback) {
this->cntl_ = callback->cntl_;
this->response_ = callback->response_;
}
~AutoReleaseClosure() override = default;
- // Will delete itself
+ // Will delete itself. all operations should be done in callback's call().
Run() only do one thing.
void Run() override {
Defer defer {[&]() { delete this; }};
- // If lock failed, it means the callback object is deconstructed, then
no need
- // to deal with the callback any more.
if (auto tmp = callback_.lock()) {
tmp->call();
}
- if (cntl_->Failed()) {
- _process_if_rpc_failed();
Review Comment:
`Run()` no longer has any fallback status handling after `callback_.lock()`.
That means every async caller must now keep a strong `shared_ptr` to the
callback alive until BRPC completion. Existing users do not all satisfy that
contract. For example, `RuntimeFilter::_push_to_remote()` keeps the callback
only in a local variable, and
`RuntimeFilterMergeControllerEntity::{send_filter_size,_send_rf_to_target}()`
pass a temporary `create_shared(...)` directly into `create_unique(...)`, so
the callback can be destroyed before the RPC even returns. In those paths
`callback_.lock()` fails here and we silently skip query cancellation / warning
logs for RPC failures and non-OK response statuses.
The new tests only invoke `callback->call()` directly, so they do not catch
this lifetime regression. Please either preserve the old `Run()`-level fallback
handling or add a strong owner for every async call site before making this
generic change.
--
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]
