Repository navigation
Reject over-limit requests on nshead server instead of CHECK - #3723
Merged
Merged
Conversation
ProcessNsheadRequest() wrapped MethodStatus::OnRequested() in CHECK(), treating a rejection of the concurrency limiter as an impossible state. Both built-in limiters really return false under overload, so the first request that reaches the limit trips the CHECK: with -crash_on_fatal_log the process aborts, and with the default configuration a FATAL log with a stack trace is emitted while the rejected request is served anyway, which silently disables the limit configured by the operator. Handle the rejection like the other protocols do: fail the controller with ELIMIT and let the handler (and the pb adaptor) answer the client. Note that OnRequested() counts the request even when it is rejected, and ConcurrencyRemover in NsheadClosure::Run() counts it out, so the concurrency accounting stays balanced. The same rejection was discarded in NsheadPbServiceAdaptor, which is used by nshead-based pb protocols (public_pbrpc, nova_pbrpc, nshead_mcpack): the method-level limiter was never enforced there. Fail with ELIMIT as well.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
No open issues 🎉
What changed in this PR
This PR fixes nshead request handling under concurrency overload by rejecting over-limit requests with ELIMIT instead of CHECK-crashing/logging fatals and inadvertently serving the request anyway.
Changes:
- Update nshead server protocol to treat
MethodStatus::OnRequested()rejection as a normal overload path and fail the controller withELIMIT. - Enforce method-level concurrency limiting in
NsheadPbServiceAdaptorby honoring the return value ofOnRequested(). - Add regression tests ensuring over-limit nshead and nshead-adapted PB requests are rejected and concurrency is properly recycled.
| File | Description |
|---|---|
src/brpc/policy/nshead_protocol.cpp |
Replace CHECK(OnRequested()) with an ELIMIT failure path under overload. |
src/brpc/nshead_pb_service_adaptor.cpp |
Stop ignoring method-level limiter rejections; fail with ELIMIT when over limit. |
test/brpc_server_unittest.cpp |
Add regression tests covering nshead service-level and nshead-adapted PB method-level concurrency rejection and slot recycling. |
Review effort: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
ProcessNsheadRequest()wrapsMethodStatus::OnRequested()inCHECK(), treating a rejection of the concurrency limiter as an impossible state. Both built-in limiters (ConstantConcurrencyLimiter,AutoConcurrencyLimiter) really return false under overload, so the first request that reaches the limit configured by the operator trips the check: with-crash_on_fatal_log=truethe process aborts, and with the default configuration a FATAL log with a stack trace is emitted while the rejected request is served anyway, i.e. the configured limit is silently disabled and the logs are flooded at the request rate. Sibling protocols (thrift, baidu_std, http, mongo) fail the controller with ELIMIT instead. The same rejection was also discarded inNsheadPbServiceAdaptor, so the method-level limit was never enforced for nshead-based pb protocols.What is changed and the side effects?
Changed:
src/brpc/policy/nshead_protocol.cpp: the service-levelMethodStatus::OnRequested()result is checked inside the existingdo/whilevalidation block and a rejection fails the controller withELIMIT, the same way as the other checks of that block, so the user handler (or the pb adaptor) answers the client instead of the request being served. The concurrency accounting stays balanced:OnRequested()counts the request even when it is rejected, andConcurrencyRemoverinNsheadClosure::Run()counts it out.src/brpc/nshead_pb_service_adaptor.cpp: the method-levelOnRequested()result is no longer ignored, an over-limit request fails withELIMIT(pbdone->statusis set before the check, soOnResponded()is still called on the failure path).test/brpc_server_unittest.cpp: two regression tests, one for a plainNsheadServiceand one for a pb service served throughPublicPbrpcServiceAdaptor. Each test holds the only concurrency slot with one request and verifies that the second request is rejected withELIMITand that the slot is recycled afterwards, so later requests are served again.Side effects:
Performance effects: none, the check is called at the same place with the same cost.
Breaking backward compatibility: none at the API level. Requests that reach a configured concurrency limit on nshead services are now answered with
ELIMITinstead of being served (with a FATAL log, or aborting the process when-crash_on_fatal_log=true), which is the documented behavior ofmax_concurrency/method_max_concurrency.Tests run:
brpc_server_unittest(all 50 cases)brpc_public_pbrpc_protocol_unittest,brpc_nova_pbrpc_protocol_unittest,brpc_nshead_mcpack_protocol_unittest,brpc_interceptor_unittestbrpc_channel_unittest(2 pre-existing failures unrelated to this change:ChannelTest.parse_hostnameneeds network access to www.baidu.com,ChannelTest.backuprequest_selective_response_raceis flaky)Check List:
🤖 This PR was automatically created by brpc-oncall