Copilot commented on code in PR #3250:
URL: https://github.com/apache/brpc/pull/3250#discussion_r2987736172
##########
src/brpc/stream.cpp:
##########
@@ -640,16 +640,21 @@ void Stream::SendFeedback() {
}
int Stream::SetHostSocket(Socket *host_socket) {
- std::call_once(_set_host_socket_flag, [this, host_socket]() {
+ int error_code = 0;
+ std::call_once(_set_host_socket_flag, [this, host_socket, &error_code]() {
SocketUniquePtr ptr;
host_socket->ReAddress(&ptr);
// TODO add *this to host socke
if (ptr->AddStream(id()) != 0) {
- CHECK(false) << id() << " fail to add stream to host socket";
+ error_code = ptr->non_zero_error_code();
return;
}
_host_socket = ptr.release();
});
+ if (_host_socket == NULL) {
+ errno = error_code ? error_code : EFAILEDSOCKET;
+ return -1;
Review Comment:
Now that `SetHostSocket()` can return `-1` instead of aborting, it’s
important that all call sites check the return value to avoid later
`CHECK(_host_socket != NULL)`/null-deref paths (e.g. `WriteToHostSocket`,
`SetConnected`). Please audit/update remaining callers that still ignore the
return value.
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -360,7 +360,19 @@ void SendRpcResponse(int64_t correlation_id, Controller*
cntl,
Stream* s = (Stream *) stream_ptr->conn();
StreamSettings *stream_settings = meta.mutable_stream_settings();
s->FillSettings(stream_settings);
- s->SetHostSocket(sock);
+ if (s->SetHostSocket(sock) != 0) {
+ const int errcode = sock->non_zero_error_code();
+ LOG_IF(WARNING, errcode != EPIPE)
+ << "Fail to bind response stream=" << response_stream_id
+ << " to " << sock->description() << ": "
+ << berror(errcode);
+ cntl->SetFailed(errcode, "Fail to bind response stream to %s",
+ sock->description().c_str());
+ Stream::SetFailed(response_stream_ids, errcode,
+ "Fail to bind response stream to %s",
+ sock->description().c_str());
Review Comment:
These new `Stream::SetFailed(..., "...%s", ...)` calls assume
`Stream::SetFailed` correctly forwards printf-style varargs. In
`src/brpc/stream.cpp`, `Stream::SetFailed` currently passes a `va_list` into
`Stream::Close(...)` via `...`, which is undefined behavior when the format
string has specifiers. This can corrupt the failure reason or crash on some
ABIs. Please fix `Stream::SetFailed/Close` to properly accept/forward a
`va_list` (or pre-format the message into a string and pass it as "%s").
```suggestion
"Fail to bind response stream to host
socket");
```
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -426,7 +447,21 @@ void SendRpcResponse(int64_t correlation_id, Controller*
cntl,
SocketUniquePtr extra_stream_ptr;
if (Socket::Address(extra_stream_id, &extra_stream_ptr) == 0) {
Stream* extra_stream = (Stream *) extra_stream_ptr->conn();
- extra_stream->SetHostSocket(sock);
+ if (extra_stream->SetHostSocket(sock) != 0) {
+ const int errcode = sock->non_zero_error_code();
+ LOG_IF(WARNING, errcode != EPIPE)
+ << "Fail to bind response stream=" << extra_stream_id
+ << " to " << sock->description() << ": "
+ << berror(errcode);
+ cntl->SetFailed(errcode, "Fail to bind response stream to
%s",
+ sock->description().c_str());
+ StreamIds remaining_stream_ids(response_stream_ids.begin()
+ i,
+ response_stream_ids.end());
+ Stream::SetFailed(remaining_stream_ids, errcode,
+ "Fail to bind response stream to %s",
+ sock->description().c_str());
Review Comment:
Same varargs-forwarding issue applies here: the formatted
`Stream::SetFailed(remaining_stream_ids, ..., "...%s", ...)` relies on
undefined behavior in `Stream::SetFailed/Close` when the format string contains
specifiers. Please address the varargs forwarding (or pre-format the message
before calling `Stream::SetFailed`).
##########
src/brpc/stream.cpp:
##########
@@ -640,16 +640,21 @@ void Stream::SendFeedback() {
}
int Stream::SetHostSocket(Socket *host_socket) {
- std::call_once(_set_host_socket_flag, [this, host_socket]() {
+ int error_code = 0;
+ std::call_once(_set_host_socket_flag, [this, host_socket, &error_code]() {
SocketUniquePtr ptr;
host_socket->ReAddress(&ptr);
// TODO add *this to host socke
Review Comment:
`Stream::SetHostSocket()` uses `std::call_once` with a stack-local
`error_code` captured by reference. Only the thread that executes the
`call_once` lambda can update `error_code`; other concurrent callers (or later
callers after a failed first attempt) will see `error_code == 0` and fall back
to `EFAILEDSOCKET`, losing the real failure reason. Consider persisting the
first failure code in a `Stream` member (e.g. `_set_host_socket_ec`) set inside
the `call_once` body and using that to set `errno` for all callers.
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -360,7 +360,19 @@ void SendRpcResponse(int64_t correlation_id, Controller*
cntl,
Stream* s = (Stream *) stream_ptr->conn();
StreamSettings *stream_settings = meta.mutable_stream_settings();
s->FillSettings(stream_settings);
- s->SetHostSocket(sock);
+ if (s->SetHostSocket(sock) != 0) {
+ const int errcode = sock->non_zero_error_code();
+ LOG_IF(WARNING, errcode != EPIPE)
+ << "Fail to bind response stream=" << response_stream_id
+ << " to " << sock->description() << ": "
+ << berror(errcode);
+ cntl->SetFailed(errcode, "Fail to bind response stream to %s",
+ sock->description().c_str());
Review Comment:
On `SetHostSocket()` failure you’re logging/propagating
`sock->non_zero_error_code()`, but `SetHostSocket()` already sets `errno` to
the specific bind failure. Using `errno` from the failing call will be more
accurate and avoids relying on the socket’s racy `_error_code` publication
timing (see comment in `Socket::non_zero_error_code()`).
--
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]
