Skip to content

Commit eed1207

Browse files
feng-yclaude
andcommitted
Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp
In SendRpcResponse, ConcurrencyRemover was destroyed before CallAfterRpcResp was called, meaning concurrency control didn't cover the after-response callback. This could lead to inaccurate concurrency tracking and latency measurements. This change adds FLAGS_concurrency_remover_manages_after_rpc_resp (default: false) and automatically sets it to controller when set_after_rpc_resp_fn is called. Implementation: - Add _concurrency_remover_manages_after_rpc_resp flag to Controller - In set_after_rpc_resp_fn(), read gflag value and set to controller instance - In baidu_rpc_protocol, use controller flag instead of global gflag - Use unique_ptr with explicit reset() for clear control flow When false (default): Original behavior - ConcurrencyRemover is released before CallAfterRpcResp via explicit reset(). When true (gflag enabled when callback set): ConcurrencyRemover lives until the end of BRPC_SCOPE_EXIT, covering the entire response lifecycle. Note: HTTP protocol not modified in this change due to its more complex async flow. Can be addressed separately if needed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent a47e349 commit eed1207

3 files changed

Lines changed: 30 additions & 4 deletions

File tree

src/brpc/controller.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ const Controller* GetSubControllerOfSelectiveChannel(
127127

128128
DECLARE_bool(usercode_in_pthread);
129129
DECLARE_bool(usercode_in_coroutine);
130+
DECLARE_bool(concurrency_remover_manages_after_rpc_resp);
130131
static const int MAX_RETRY_COUNT = 1000;
131132
static bvar::Adder<int64_t>* g_ncontroller = NULL;
132133

@@ -298,6 +299,7 @@ void Controller::ResetPods() {
298299
_response_streams.clear();
299300
_remote_stream_settings = NULL;
300301
_auth_flags = 0;
302+
_concurrency_remover_manages_after_rpc_resp = false;
301303
_rpc_received_us = 0;
302304
}
303305

@@ -1593,6 +1595,12 @@ int Controller::GetSockOption(int level, int optname, void* optval, socklen_t* o
15931595
}
15941596
}
15951597

1598+
void Controller::set_after_rpc_resp_fn(AfterRpcRespFnType&& fn) {
1599+
_after_rpc_resp_fn = fn;
1600+
// Set the flag from global gflag when after_rpc_resp_fn is set
1601+
_concurrency_remover_manages_after_rpc_resp = FLAGS_concurrency_remover_manages_after_rpc_resp;
1602+
}
1603+
15961604
void Controller::CallAfterRpcResp(const google::protobuf::Message* req, const google::protobuf::Message* res) {
15971605
if (_after_rpc_resp_fn) {
15981606
_after_rpc_resp_fn(this, req, res);

src/brpc/controller.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -621,10 +621,15 @@ friend void policy::ProcessThriftRequest(InputMessageBase*);
621621
const google::protobuf::Message* req,
622622
const google::protobuf::Message* res)>;
623623

624-
void set_after_rpc_resp_fn(AfterRpcRespFnType&& fn) { _after_rpc_resp_fn = fn; }
624+
void set_after_rpc_resp_fn(AfterRpcRespFnType&& fn);
625625

626626
void CallAfterRpcResp(const google::protobuf::Message* req, const google::protobuf::Message* res);
627627

628+
// Check whether ConcurrencyRemover should manage the lifecycle of CallAfterRpcResp.
629+
bool concurrency_remover_manages_after_rpc_resp() const {
630+
return _concurrency_remover_manages_after_rpc_resp;
631+
}
632+
628633
void set_request_content_type(ContentType type) {
629634
_request_content_type = type;
630635
}
@@ -921,6 +926,7 @@ friend void policy::ProcessThriftRequest(InputMessageBase*);
921926
uint32_t _auth_flags;
922927

923928
AfterRpcRespFnType _after_rpc_resp_fn;
929+
bool _concurrency_remover_manages_after_rpc_resp;
924930

925931
// The point in time when the rpc is read from the socket
926932
int64_t _rpc_received_us;

src/brpc/policy/baidu_rpc_protocol.cpp

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,11 @@ DEFINE_bool(baidu_protocol_use_fullname, true,
6262
DEFINE_bool(baidu_std_protocol_deliver_timeout_ms, false,
6363
"If this flag is true, baidu_std puts timeout_ms in requests.");
6464

65+
DEFINE_bool(concurrency_remover_manages_after_rpc_resp, false,
66+
"If this flag is true, ConcurrencyRemover will manage the lifecycle "
67+
"of CallAfterRpcResp, ensuring concurrency control covers the entire "
68+
"response processing including after-response callbacks.");
69+
6570
DECLARE_bool(pb_enum_as_number);
6671

6772
// Notes:
@@ -285,9 +290,14 @@ void SendRpcResponse(int64_t correlation_id, Controller* cntl,
285290

286291
// Recycle resources at the end of this function.
287292
BRPC_SCOPE_EXIT {
288-
{
289-
// Remove concurrency and record latency at first.
290-
ConcurrencyRemover concurrency_remover(method_status, cntl, received_us);
293+
std::unique_ptr<ConcurrencyRemover> concurrency_remover_ptr(
294+
new ConcurrencyRemover(method_status, cntl, received_us));
295+
296+
// Only manage CallAfterRpcResp lifecycle if the flag is set
297+
// (which happens when set_after_rpc_resp_fn is called with the gflag enabled)
298+
if (!cntl->concurrency_remover_manages_after_rpc_resp()) {
299+
// Original behavior: remove concurrency before CallAfterRpcResp
300+
concurrency_remover_ptr.reset();
291301
}
292302

293303
std::unique_ptr<Controller, LogErrorTextAndDelete> recycle_cntl(cntl);
@@ -302,6 +312,8 @@ void SendRpcResponse(int64_t correlation_id, Controller* cntl,
302312
} else {
303313
BaiduProxyPBMessages::Return(static_cast<BaiduProxyPBMessages*>(messages));
304314
}
315+
// If concurrency_remover_manages_after_rpc_resp() is true,
316+
// concurrency_remover_ptr will be destroyed here
305317
};
306318

307319
StreamIds response_stream_ids = accessor.response_streams();

0 commit comments

Comments
 (0)