Copilot commented on code in PR #3407:
URL: https://github.com/apache/brpc/pull/3407#discussion_r3649529999
##########
src/brpc/controller.h:
##########
@@ -260,6 +260,22 @@ friend void
policy::ProcessThriftRequest(InputMessageBase*);
// Set checksum type for request.
void set_request_checksum_type(ChecksumType t) { _request_checksum_type =
t; }
+ // Whether the request's checksum (see set_request_checksum_type) also
+ // covers the attachment, in addition to the serialized body. Defaults
+ // to false (checksum covers body only), preserving the pre-existing
+ // behavior. This setting is sent to the peer along with the request so
+ // that it recomputes the checksum over the same range; it is meaningless
+ // (and rejected, see baidu_rpc_protocol.cpp) together with
+ // request_will_be_read_progressively() since the attachment is not
+ // fully buffered before the checksum must be verified.
Review Comment:
The comment says the setting is "rejected" when progressive attachment
reading is enabled (and references baidu_rpc_protocol.cpp), but the current
implementation actually just skips folding the attachment into the checksum in
that case. This mismatch can mislead API users; update the comment to match the
real behavior (or implement actual rejection).
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -349,6 +358,9 @@ void SendRpcResponse(int64_t correlation_id, Controller*
cntl,
meta.set_content_type(cntl->response_content_type());
meta.set_checksum_type(cntl->response_checksum_type());
meta.set_checksum_value(accessor.checksum_value());
+ if (cntl->response_checksum_attachment()) {
+ meta.set_checksum_with_attachment(true);
+ }
Review Comment:
`meta.checksum_with_attachment` is set whenever
`response_checksum_attachment()` is true, but `SerializeResponse()` may skip
folding the attachment into the checksum when
`is_response_read_progressively()` is set. In that case the peer will verify
body+attachment (because the meta flag is true) against a body-only checksum,
guaranteeing checksum failure when the attachment is non-empty. Gate the meta
flag on the same condition used to actually include the attachment in the
checksum (or reject the combination).
This issue also appears on line 1106 of the same file.
--
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]
