langke1988 commented on PR #2953:
URL: https://github.com/apache/thrift/pull/2953#issuecomment-2028803738
I'm sorry for the confusion caused. First of all, thank you for your
response. Perhaps my try-catch block was not placed properly, which led to your
confusion. In fact, I was throwing an exception to catch the highlighted area
in red, not the green one.
Please kindly analyze the TSocket::write_partial function. My English is not
very good and my expression may not be clear. I apologize for any confusion and
appreciate your help.
void TSocket::write(const uint8_t* buf, uint32_t len) {
uint32_t sent = 0;
try{
while (sent < len) {
uint32_t b = write_partial(buf + sent, len - sent);
if (b == 0) {
// This should only happen if the timeout set with SO_SNDTIMEO
expired.
// Raise an exception.
throw TTransportException(TTransportException::TIMED_OUT, "send
timeout expired");
}
sent += b;
}
}
catch(TTransportException e) {
throw e;
}
}
There are uncaught exceptions in the TSocket::write method. Specifically, if
the return value of the write_partial method is less than 0, it will enter the
exception handling code block. In the exception handling code block, different
types of exceptions are thrown based on different error types, but these
exceptions are not caught and are continued to be thrown to the caller for
handling. Therefore, these exceptions should be caught in the write method to
properly handle exceptional situations.
uint32_t TSocket::write_partial(const uint8_t* buf, uint32_t len) {
if (socket_ == THRIFT_INVALID_SOCKET) {
throw TTransportException(TTransportException::NOT_OPEN, "Called write
on non-open socket");
}
uint32_t sent = 0;
int flags = 0;
#ifdef MSG_NOSIGNAL
// Note the use of MSG_NOSIGNAL to suppress SIGPIPE errors, instead we
// check for the THRIFT_EPIPE return condition and close the socket in
that case
flags |= MSG_NOSIGNAL;
#endif // ifdef MSG_NOSIGNAL
int b = static_cast<int>(send(socket_, const_cast_sockopt(buf + sent), len
- sent, flags));
if (b < 0) {
if (THRIFT_GET_SOCKET_ERROR == THRIFT_EWOULDBLOCK ||
THRIFT_GET_SOCKET_ERROR == THRIFT_EAGAIN) {
return 0;
}
// Fail on a send error
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::write_partial() send() " +
getSocketInfo(), errno_copy);
if (errno_copy == THRIFT_EPIPE || errno_copy == THRIFT_ECONNRESET
|| errno_copy == THRIFT_ENOTCONN) {
throw TTransportException(TTransportException::NOT_OPEN, "write()
send()", errno_copy);
}
throw TTransportException(TTransportException::UNKNOWN, "write()
send()", errno_copy);
}
// Fail on blocked send
if (b == 0) {
throw TTransportException(TTransportException::NOT_OPEN, "Socket send
returned 0.");
}
return b;
}
---- Replied Message ----
| From | Jens ***@***.***> |
| Date | 3/31/2024 22:01 |
| To | ***@***.***> |
| Cc | ***@***.***>,
***@***.***> |
| Subject | Re: [apache/thrift] fix 1 bug about TSocket::write (PR #2953) |
It makes absolutely no sense to throw an exception, immediately catch it and
w/o any other action simply re-throwing it. The only thing you achieve is to
make your "high availability server" run slower. It will not change anything in
the behaviour. You have a test case ready to prove your claim?
Please make sure to submit this modification
The more you trying to push decibels instead of providing sound arguments
the less likely it is.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
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]
