chenBright commented on code in PR #3250:
URL: https://github.com/apache/brpc/pull/3250#discussion_r3205972732
##########
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 = errno;
+ 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());
+ return;
Review Comment:
I think that if binding the response stream fails, we shouldn't just return
directly; we should still try writing a response.
##########
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 = errno;
+ 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());
Review Comment:
Just call `cntl->SetFailed` once.
##########
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 = errno;
Review Comment:
Why use errno as the reason for SetHostSocket failure? SetHostSocket failure
does not set errno.
--
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]
