You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "langke1988 (via GitHub)" <gi...@apache.org> on 2024/03/30 09:03:13 UTC
[PR] fix 1 bug about TSocket::write [thrift]
langke1988 opened a new pull request, #2953:
URL: https://github.com/apache/thrift/pull/2953
<!-- Explain the changes in the pull request below: -->
TSocket::write should catch write_partial's exception and throw it
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix 1 bug about TSocket::write [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G closed pull request #2953: fix 1 bug about TSocket::write
URL: https://github.com/apache/thrift/pull/2953
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix 1 bug about TSocket::write [thrift]
Posted by "langke1988 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix 1 bug about TSocket::write [thrift]
Posted by "langke1988 (via GitHub)" <gi...@apache.org>.
langke1988 commented on PR #2953:
URL: https://github.com/apache/thrift/pull/2953#issuecomment-2028717854
Please make sure to submit this modification. TSocket::write call write_partial, which may throw TTransportException. Therefore, TSocket::write must catch and throw this exception, otherwise the following issues may occur:
I have multiple Thrift servers, and clients connect to the server each time by catching exceptions to determine if the server is available, thereby achieving high availability. However, TSocket::write did not throw this exception, causing my high availability to fail. After modifying this code, the problem disappeared.
---- Replied Message ----
| From | Jens ***@***.***> |
| Date | 3/31/2024 01:03 |
| To | ***@***.***> |
| Cc | ***@***.***>,
***@***.***> |
| Subject | Re: [apache/thrift] fix 1 bug about TSocket::write (PR #2953) |
Closed #2953.
—
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix 1 bug about TSocket::write [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #2953:
URL: https://github.com/apache/thrift/pull/2953#issuecomment-2031943290
First, essentially your "solution" boils down to
```
void TSocket::write(const uint8_t* buf, uint32_t len) {
try{
// some code that may throw TTransportException (or something else)
}
catch(TTransportException e) { // catch it
throw e; // re-throw it
}
}
```
It's probably me but I don't see the point in this.
Second, whenever you call code, be it your own or 3rd party code, there may be errors that you don't expect. You may run out of disk space, memory or run into communication timeouts. Your point was that this is to ensure your server runs with high availability:
> clients connect to the server each time by catching exceptions to determine if the server is available
I can't see how catching one particular exception at this place will help with that. Why can't you catch and handle the exception in the calling code? And how does catching and re-throwing help with making the server more available? Also, why is it clients that have to perform that task? Assumed you use some state-of-the-art system like e.g. kubernetes, [there is special handling built right into the platform for exactly this task](https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/) and therefore no need to re-invent the wheel again.
To me it looks as if you might try to solve the wrong problem, or at least at the wrong place.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix 1 bug about TSocket::write [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #2953:
URL: https://github.com/apache/thrift/pull/2953#issuecomment-2028099942
> TSocket::write should catch write_partial's exception and throw it
... because .... ?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix 1 bug about TSocket::write [thrift]
Posted by "langke1988 (via GitHub)" <gi...@apache.org>.
langke1988 commented on PR #2953:
URL: https://github.com/apache/thrift/pull/2953#issuecomment-2033380539
Thank you for your patient explanation. It seems that I was mistaken in my thinking, and I apologize for any confusion I may have caused you.
---- Replied Message ----
| From | Jens ***@***.***> |
| Date | 4/2/2024 20:43 |
| To | ***@***.***> |
| Cc | ***@***.***>,
***@***.***> |
| Subject | Re: [apache/thrift] fix 1 bug about TSocket::write (PR #2953) |
First, essentially your "solution" boils down to
void TSocket::write(const uint8_t* buf, uint32_t len) {
try{
// some code that may throw TTransportException (or something else)
}
catch(TTransportException e) { // catch it
throw e; // re-throw it
}
}
It's probably me but I don't see the point in this.
Second, whenever you call code, be it your own or 3rd party code, there may be errors that you don't expect. You may run out of disk space, memory or run into communication timeouts. Your point was that this is to ensure your server runs with high availability:
clients connect to the server each time by catching exceptions to determine if the server is available
I can't see how catching one particular exception at this place will help with that. Why can't you catch and handle the exception in the calling code? And how does catching and re-throwing help with making the server more available? Also, why is it clients that have to perform that task? Assumed you use some state-of-the-art system like e.g. kubernetes, there is special handling built right into the platform for exactly this task and therefore no need to re-invent the wheel again.
To me it looks as if you might try to solve the wrong problem, or at least at the wrong place.
—
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org