Repository navigation
Validate RPC response sockets - #3580
Merged
Merged
Conversation
Check that responses arrive on the socket used by the matching request attempt before updating the controller. Account for current attempts and unfinished backup requests across the four protobuf RPC protocols. Add Hulu response tests for matching and mismatched sockets while keeping direct response injection compatible with existing internal callers.
Verify rejection and subsequent completion on the sending socket for baidu_std, sofa_pbrpc, and public_pbrpc response handlers. Exercise real channel/server calls across all four affected protocols and their supported connection types using an ephemeral server port.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unversioned responses can still complete real requests, and rejected streaming responses omit required stream cleanup.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds socket validation to prevent mismatched or stale protobuf RPC responses from updating controllers.
Changes:
- Validates response socket and request-attempt correlation IDs.
- Drops mismatched responses while preserving valid backup attempts.
- Adds protocol-level and real RPC regression coverage.
| File | Description |
|---|---|
src/brpc/details/controller_private_accessor.h |
Adds attempt/socket matching logic. |
src/brpc/policy/baidu_rpc_protocol.cpp |
Validates baidu_std responses. |
src/brpc/policy/hulu_pbrpc_protocol.cpp |
Validates Hulu responses. |
src/brpc/policy/sofa_pbrpc_protocol.cpp |
Validates SOFA responses. |
src/brpc/policy/public_pbrpc_protocol.cpp |
Validates public_pbrpc responses. |
test/brpc_channel_unittest.cpp |
Tests baidu_std and real RPC exchanges. |
test/brpc_hulu_pbrpc_protocol_unittest.cpp |
Tests mismatches, retries, and backups. |
test/brpc_sofa_pbrpc_protocol_unittest.cpp |
Tests SOFA socket matching. |
test/brpc_public_pbrpc_protocol_unittest.cpp |
Tests public_pbrpc socket matching. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Accept the base correlation ID only for direct response injection without a sending socket. Real requests must match a versioned attempt ID. Reset all advertised streams on the arrival socket when rejecting a baidu_std response. Extend the regression test to check both rejection paths and successful completion by the valid response.
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?
Problem Summary:
The protobuf RPC response handlers associate replies with a controller using
the correlation ID without checking the connection used by the matching
request attempt. A reply arriving on another connection can update that
controller, and a stale retry reply can update the response before being
ignored by the call-completion path.
What is changed and the side effects?
Changed:
controller in baidu_std, hulu_pbrpc, sofa_pbrpc, and public_pbrpc.
replies and release the correlation ID lock.
completion after a dropped reply, stale retry replies, and unfinished
backup attempts.
supported single, pooled, and short connection types on an ephemeral port.
Side effects:
per response.
sending connection. Direct response injection without a sending socket
remains supported for existing internal callers.
Check List: