Fix bthread_id lock leak in ProcessNsheadMcpackResponse - #3574
Merged
Merged
Conversation
After ProcessNsheadMcpackResponse acquires the bthread_id lock, its two
early-return paths (response object missing, mcpack parse failure) skip
accessor.OnResponse(), the only unlock entry. The leaked lock makes
Join(correlation_id) hang forever and the RPC timeout cannot rescue it
since the timeout error is only consumed during unlocking. A server
returning a valid nshead header plus a malformed mcpack body reliably
triggers this.
Adopt the do { ... break ... } while(0) pattern (consistent with
ProcessRpcResponse) so that OnResponse() is always reached.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The production fix is sound and well covered; only minor test-fixture descriptor cleanup remains.
Review effort: Balanced
Findings: 1
What changed in this PR
Prevents nshead-mcpack response handling from leaking correlation-ID locks on error paths.
Changes:
- Ensures all post-lock paths call
OnResponse. - Adds coverage for malformed, unregistered, missing-response, and successful cases.
| File | Description |
|---|---|
src/brpc/policy/nshead_mcpack_protocol.cpp |
Consolidates response completion and lock release. |
test/brpc_nshead_mcpack_protocol_unittest.cpp |
Tests lock release across response paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Descriptor::full_name() returns absl::string_view since protobuf 35.x, which cannot be implicitly converted to the const std::string& parameter of mcpack2pb::register_message_handler_or_die. Wrap it into std::string explicitly.
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 #N/A
Problem Summary:
In
ProcessNsheadMcpackResponse(brpc/policy/nshead_mcpack_protocol.cpp), afterbthread_id_lock(cid, &cntl)succeeds, there were two early-return paths that skippedaccessor.OnResponse(cid, saved_error), which is the only place that unlocks the correlation_id:cntl->response() == nullptr(no response object)handler.parse_from_iobuf(...)fails (malformed mcpack body), which didreturn cntl->CloseConnection(...)Once the lock leaks, the synchronous RPC caller hangs forever in
Join(correlation_id), async calls never rundone->Run(), and even the RPC timeout cannot rescue the call:HandleTimeoutonly enqueues the error intopending_q, which is consumed exclusively insidebthread_id_unlock. A server that returns a valid nshead header plus a body that cannot be parsed as mcpack reliably triggers this, permanently hanging one client bthread per affected call.What is changed and the side effects?
Changed:
ProcessNsheadMcpackResponsenow follows the samedo { ... break ... } while (0)pattern used byProcessRpcResponseand other protocol handlers: all paths (missing response object, mcpack parse failure, success) fall through tomsg.reset()andaccessor.OnResponse(cid, saved_error), so the bthread_id lock is always released.test/brpc_nshead_mcpack_protocol_unittest.cppcovering: malformed mcpack body, unregistered message handler, missing response object, and the success path. Each error-path case asserts that the correlation_id is no longer locked (a leaked lock would makebrpc::Joinhang forever). Without the fix these three cases fail; with the fix all four pass.Side effects:
Performance effects: none; the change only restructures control flow after response parsing.
Breaking backward compatibility: none. RPCs whose response body cannot be parsed now fail the controller with the existing
ECLOSEerror (fromCloseConnection) and return to the caller instead of hanging forever.Check List:
cmake -S . -B build -DBUILD_UNIT_TESTS=ON+make -j6.brpc_nshead_mcpack_protocol_unittest(new, 4 cases, verified failing before the fix and passing after); regression-checkedbrpc_mcpack2pb_unittest,brpc_nova_pbrpc_protocol_unittest,brpc_esp_protocol_unittest,brpc_sofa_pbrpc_protocol_unittest— all pass.🤖 This PR was automatically created by brpc-oncall