Copilot commented on code in PR #3386:
URL: https://github.com/apache/brpc/pull/3386#discussion_r3585550645
##########
src/brpc/stream.cpp:
##########
@@ -672,7 +672,7 @@ int Stream::SetHostSocket(Socket *host_socket) {
}
_host_socket = ptr.release();
});
- return 0;
+ return _host_socket != NULL ? 0 : -1;
Review Comment:
`SetHostSocket()` now returns `-1` when the host-socket binding did not
succeed, but several call sites currently ignore the return value (e.g.
`policy/baidu_rpc_protocol.cpp:363,429` and `controller.cpp:1521`). Those paths
can still proceed with `_host_socket == NULL`, which later trips
`CHECK(_host_socket != NULL)` in other Stream methods.
Consider enforcing handling by marking the declaration `int
SetHostSocket(Socket*) WARN_UNUSED_RESULT;` (and/or updating call sites to
check/propagate the error).
##########
src/brpc/stream.cpp:
##########
@@ -672,7 +672,7 @@ int Stream::SetHostSocket(Socket *host_socket) {
}
_host_socket = ptr.release();
});
- return 0;
+ return _host_socket != NULL ? 0 : -1;
Review Comment:
This change introduces a new observable behavior: `SetHostSocket()` may now
fail and the caller is expected to handle it. There are existing streaming unit
tests (`test/brpc_streaming_rpc_unittest.cpp`), but none appear to cover the
`AddStream()` failure path that leaves `_host_socket` unset.
Adding a regression test that forces `Socket::AddStream()` to fail (e.g. by
failing the socket before binding) and asserting `SetHostSocket()` returns `-1`
would help prevent reintroducing the original crash scenario.
--
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]
