You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "feng-y (via GitHub)" <gi...@apache.org> on 2026/05/25 09:55:01 UTC
[PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
feng-y opened a new pull request, #3309:
URL: https://github.com/apache/brpc/pull/3309
## Problem
In `SendRpcResponse`, `ConcurrencyRemover` was destroyed before `CallAfterRpcResp` was called. This means the concurrency control didn't cover the after-response callback, which could lead to:
- Inaccurate concurrency tracking
- Incorrect latency measurements
- Premature resource release if callbacks hold resources
## Solution
Add `FLAGS_concurrency_remover_manages_after_rpc_resp` (default: `false`) to optionally extend `ConcurrencyRemover`'s lifetime to cover `CallAfterRpcResp`.
### Behavior
- **flag=false (default)**: Original behavior - `ConcurrencyRemover` is released before `CallAfterRpcResp` via explicit `reset()`
- **flag=true**: `ConcurrencyRemover` lives until the end of `BRPC_SCOPE_EXIT`, covering the entire response lifecycle including after-response callbacks
### Implementation
Uses `unique_ptr` with explicit `reset()` for clear control flow:
- Always allocate `ConcurrencyRemover` via `unique_ptr`
- Default path: explicitly `reset()` before `CallAfterRpcResp`
- Extended path: let `unique_ptr` naturally destruct at scope end
## Backward Compatibility
✅ Default behavior unchanged (flag defaults to `false`)
✅ No impact on existing deployments
✅ Opt-in feature for users who need accurate concurrency tracking
## Testing
Tested with:
- Default flag value (original behavior preserved)
- Flag enabled (concurrency properly managed through `CallAfterRpcResp`)
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "feng-y (via GitHub)" <gi...@apache.org>.
feng-y commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4550876687
I've updated the implementation based on the feedback:
## Current Implementation
### ✅ What Changed
1. **Kept the gflag**: `FLAGS_concurrency_remover_manages_after_rpc_resp` (default: false)
2. **Capture to controller**: In `set_after_rpc_resp_fn()`, the gflag value is captured to the controller instance
3. **Protocol uses controller flag**: `baidu_rpc_protocol` now uses `cntl->concurrency_remover_manages_after_rpc_resp()` instead of the global gflag
### 🎯 How It Works
```cpp
// 1. User optionally sets global gflag
// -concurrency_remover_manages_after_rpc_resp=true
// 2. User sets callback (in service implementation)
controller.set_after_rpc_resp_fn([](Controller* cntl, ...) {
// At this point, the gflag value is captured to controller
// _concurrency_remover_manages_after_rpc_resp = FLAGS_concurrency_remover_manages_after_rpc_resp
});
// 3. In SendRpcResponse
if (!cntl->concurrency_remover_manages_after_rpc_resp()) {
concurrency_remover_ptr.reset(); // Release early (default)
}
```
### 📝 Scope
- **Modified**: `baidu_rpc_protocol` only
- **Not modified**: `http_rpc_protocol`
- HTTP has a more complex async flow (`HttpResponseSenderAsDone`)
- Can be addressed separately if needed
- Wanted to keep this PR focused and low-risk
### ✅ Benefits
- Global configuration via gflag
- Per-controller instance capture (no race conditions)
- Backward compatible (default behavior unchanged)
- Minimal changes (only 3 files, +19 -3 lines)
Let me know if you'd like me to also update HTTP protocol, or if we should handle that separately!
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "feng-y (via GitHub)" <gi...@apache.org>.
feng-y commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4550826062
Thanks for the feedback! I've updated the implementation based on your suggestions:
## Changes Made
1. **Removed global gflag** - Now uses a controller-level flag instead
2. **Automatic flag setting** - The flag is automatically set to `true` when `set_after_rpc_resp_fn()` is called
3. **Updated HTTP protocol** - Applied the same logic to `http_rpc_protocol.cpp`
## How It Works
When a user sets an after-response callback:
```cpp
controller.set_after_rpc_resp_fn([](Controller* cntl, const Message* req, const Message* res) {
// Custom callback logic
});
// Flag is automatically set to true here
```
The `ConcurrencyRemover` will then automatically manage the lifecycle to cover the callback execution.
## Benefits
- ✅ No global configuration needed
- ✅ Automatic opt-in based on usage
- ✅ Backward compatible (default behavior unchanged)
- ✅ Consistent across both baidu_rpc and http protocols
Please let me know if there are any other concerns!
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4533988269
Looking at the previous code, concurrency_remover was executed only after all responses had finished executing. Therefore, I think it's sufficient to keep it consistent with the initial code; there's no need to add a flag.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3309:
URL: https://github.com/apache/brpc/pull/3309#discussion_r3308815281
##########
src/brpc/controller.cpp:
##########
@@ -1593,6 +1595,12 @@ int Controller::GetSockOption(int level, int optname, void* optval, socklen_t* o
}
}
+void Controller::set_after_rpc_resp_fn(AfterRpcRespFnType&& fn) {
+ _after_rpc_resp_fn = fn;
+ // Set the flag from global gflag when after_rpc_resp_fn is set
+ _concurrency_remover_manages_after_rpc_resp = FLAGS_concurrency_remover_manages_after_rpc_resp;
Review Comment:
Do not rely on gflag, but rely on the Controller's _flags.
https://github.com/apache/brpc/blob/f97f23edbaf3f47e25dc925a947b15b45810276a/src/brpc/controller.h#L808
https://github.com/apache/brpc/blob/f97f23edbaf3f47e25dc925a947b15b45810276a/src/brpc/controller.h#L133-L154
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "feng-y (via GitHub)" <gi...@apache.org>.
feng-y commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4570267982
Thanks. Just to confirm: with the Controller::_flags approach, set_after_rpc_resp_fn() itself becomes the opt-in signal, right?\n\nThat means we do not need an extra gflag to enable this behavior. Any RPC that registers after_rpc_resp_fn will have ConcurrencyRemover cover CallAfterRpcResp, while RPCs without the callback keep the old behavior.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4570843397
> Thanks. Just to confirm: with the Controller::_flags approach, set_after_rpc_resp_fn() itself becomes the opt-in signal, right?
No. The set_after_rpc_resp_fn() function needs an additional parameter (default value is false). Setting this parameter to true indicates that ConcurrencyRemover covers CallAfterRpcResp.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "feng-y (via GitHub)" <gi...@apache.org>.
feng-y commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4535485738
Thanks. My concern is that removing the gflag would not just drop configurability, but would also change the default semantics.\n\nConcurrencyRemover's destructor calls MethodStatus::OnResponded(...), so changing its lifetime affects not only when concurrency is released, but also the latency accounting window. If it is made to cover CallAfterRpcResp unconditionally, method_status latency would also start including the after-response callback. I think that semantic change should be evaluated explicitly as well.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4539179613
> Thanks. My concern is that removing the gflag would not just drop configurability, but would also change the default semantics.
>
> ConcurrencyRemover's destructor calls MethodStatus::OnResponded(...), so changing its lifetime affects not only when concurrency is released, but also the latency accounting window. If it is made to cover CallAfterRpcResp unconditionally, method_status latency would also start including the after-response callback. I think that semantic change should be evaluated explicitly as well.
I agree to add a flag.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add gflag to manage ConcurrencyRemover lifecycle for CallAfterRpcResp (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3309:
URL: https://github.com/apache/brpc/pull/3309#issuecomment-4550758645
> I agree to add a flag.
I think it's more appropriate to set the flag in the controller using `set_after_rpc_resp_fn`.
https://github.com/apache/brpc/blob/a47e34903fdbb202c8786167bddd5ea519772d84/src/brpc/controller.h#L624-L626
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org