github-actions[bot] commented on code in PR #61782:
URL: https://github.com/apache/doris/pull/61782#discussion_r2996234806
##########
be/src/util/brpc_closure.h:
##########
@@ -96,15 +93,15 @@ class AutoReleaseClosure : public google::protobuf::Closure
{
// Will delete itself
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();
- }
+ // shouldn't do heavy work here. all heavy work should be done in
callback's call() (which means in success/failure handlers)
if (cntl_->Failed()) {
- _process_if_rpc_failed();
+ LOG(WARNING) << "brpc failed: " << cntl_->ErrorText();
} else {
- _process_status<ResponseType>(response_.get());
+ _log_error_status<ResponseType>(response_.get());
+ }
+ // this must be the LAST operation in this function, because call()
may reuse the callback! (response_ is in callback_)
+ if (auto tmp = callback_.lock()) {
+ tmp->call();
}
Review Comment:
**Bug: Missing unconstrained fallback overload for `_log_error_status` —
will fail to compile.**
The old code had two `_process_status` overloads:
```cpp
template <typename Response>
void _process_status(Response* response) {} // no-op fallback
template <HasStatus Response>
void _process_status(Response* response) { ... } // constrained
```
The new code only has the constrained version `_log_error_status<HasStatus
Response>`, but the unconstrained no-op fallback was not carried over. This
means `_log_error_status<ResponseType>(response_.get())` will fail to compile
when `ResponseType` does not satisfy the `HasStatus` concept.
**Affected caller:** `AutoReleaseClosure<PTabletWriterCancelRequest,
DummyBrpcCallback<PTabletWriterCancelResult>>` in `vtablet_writer.cpp:1229`.
`PTabletWriterCancelResult` is an empty protobuf message with no `status()`
field.
**Fix:** Add a no-op fallback:
```cpp
template <typename Response>
void _log_error_status(Response*) {
// No status field to check — nothing to log.
}
```
C++20 concept subsumption ensures the `HasStatus`-constrained overload is
preferred when it matches.
--
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]
