wwbmmm opened a new pull request, #3574:
URL: https://github.com/apache/brpc/pull/3574
### What problem does this PR solve?
Issue Number: resolve #N/A
Problem Summary:
In `ProcessNsheadMcpackResponse` (brpc/policy/nshead_mcpack_protocol.cpp),
after `bthread_id_lock(cid, &cntl)` succeeds, there were two early-return paths
that skipped `accessor.OnResponse(cid, saved_error)`, which is the only place
that unlocks the correlation_id:
1. `cntl->response() == nullptr` (no response object)
2. `handler.parse_from_iobuf(...)` fails (malformed mcpack body), which did
`return cntl->CloseConnection(...)`
Once the lock leaks, the synchronous RPC caller hangs forever in
`Join(correlation_id)`, async calls never run `done->Run()`, and even the RPC
timeout cannot rescue the call: `HandleTimeout` only enqueues the error into
`pending_q`, which is consumed exclusively inside `bthread_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:
- `ProcessNsheadMcpackResponse` now follows the same `do { ... break ... }
while (0)` pattern used by `ProcessRpcResponse` and other protocol handlers:
all paths (missing response object, mcpack parse failure, success) fall through
to `msg.reset()` and `accessor.OnResponse(cid, saved_error)`, so the bthread_id
lock is always released.
- Added `test/brpc_nshead_mcpack_protocol_unittest.cpp` covering: 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 make `brpc::Join` hang 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 `ECLOSE` error (from
`CloseConnection`) and return to the caller instead of hanging forever.
### Check List:
- Compilable: verified with `cmake -S . -B build -DBUILD_UNIT_TESTS=ON` +
`make -j6`.
- Tests: `brpc_nshead_mcpack_protocol_unittest` (new, 4 cases, verified
failing before the fix and passing after); regression-checked
`brpc_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
--
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]
