wasphin commented on code in PR #3669:
URL: https://github.com/apache/brpc/pull/3669#discussion_r4385707470
##########
src/brpc/policy/http_rpc_protocol.cpp:
##########
@@ -420,8 +420,24 @@ void ProcessHttpResponse(InputMessageBase* msg) {
}
const std::string* grpc_status =
res_header->GetHeader(common->GRPC_STATUS);
if (grpc_status) {
- // TODO: More strict parsing
- GrpcStatus status = (GrpcStatus)strtol(grpc_status->data(),
nullptr, 10);
+ int code = 0;
+ bool valid = !grpc_status->empty();
+ for (char c : *grpc_status) {
Review Comment:
Thanks for the fix and the additional tests!
I have two suggestions:
1. **Reuse `butil::StringToInt()` instead of implementing a custom
digit-by-digit parser.**
Although `StringToInt()` accepts some representations that do not
strictly conform to the gRPC wire format (e.g., leading zeros, leading signs,
or whitespace), I think this is a reasonable trade-off for compatibility. The
important part is to reject malformed inputs and integer overflow rather than
silently interpreting them as `GRPC_OK`.
Strict RFC compliance and practical compatibility are not necessarily the
same thing. Unless strict validation provides a concrete benefit, I'd prefer
reusing the existing, well-tested utility to keep the implementation simpler.
2. **Handle unknown status codes according to the gRPC specification.**
The [gRPC status-code
specification](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md)]
states that implementations encountering unrecognized status codes must either
propagate them or convert them to `UNKNOWN`.
Therefore, a successfully parsed integer such as `17` should not be
treated as a malformed response simply because it falls outside the currently
defined range (0–16).
I'd suggest distinguishing conversion failures (e.g., `"invalid"`,
`"0junk"`, or overflow) from successfully parsed but unknown status codes,
mapping the latter to `GRPC_UNKNOWN` or preserving their original values.
These changes would simplify the implementation while improving
compatibility and forward compatibility.
--
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]
