You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by ctubbsii <gi...@git.apache.org> on 2017/02/10 03:58:05 UTC
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
GitHub user ctubbsii opened a pull request:
https://github.com/apache/thrift/pull/1186
THRIFT-1805 Provide option for handling RTEs
Adds a Java option to the generator to generate code which lets Thrift
handle RuntimeExceptions from a service, and present them as
TApplicationException to the client.
This addresses a back-and-forth debate over the expected behavior of
this in Java services, and the resulting breakage, as described in
THRIFT-1805 and related issues, by making the behavior configurable.
Throwable types are not handled, because it is generally ill-advised to
handle Error types within a Java application. So, this only allows
configuration of handling RuntimeException.
This patch preserves current behavior as the default, and works with
previously generated code.
This patch does not affect AsyncProcessFunction exception handling.
Related JIRAs: THRIFT-378, THRIFT-1658, THRIFT-1805, THRIFT-3607
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ctubbsii/thrift handle-runtimeexceptions
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1186.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1186
----
commit 48ee27b210edf2d92af2f5060820d77ca0898aff
Author: Christopher Tubbs <ct...@apache.org>
Date: 2017-02-10T03:32:00Z
THRIFT-1805 Provide option for handling RTEs
Adds a Java option to the generator to generate code which lets Thrift
handle RuntimeExceptions from a service, and present them as
TApplicationException to the client.
This addresses a back-and-forth debate over the expected behavior of
this in Java services, and the resulting breakage, as described in
THRIFT-1805 and related issues, by making the behavior configurable.
Throwable types are not handled, because it is generally ill-advised to
handle Error types within a Java application. So, this only allows
configuration of handling RuntimeException.
This patch preserves current behavior as the default, and works with
previously generated code.
This patch does not affect AsyncProcessFunction exception handling.
Related JIRAs: THRIFT-378, THRIFT-1658, THRIFT-1805, THRIFT-3607
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100911117
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -5265,6 +5274,9 @@ THRIFT_REGISTER_GENERATOR(
" android_legacy: Do not use java.io.IOException(throwable) (available for Android 2.3 and "
"above).\n"
" option_type: Wrap optional fields in an Option type.\n"
+ " handle_runtime_exceptions:\n"
+ " Generated services will handle RuntimeException as "
--- End diff --
I don't think we need to mention anything about the client side w.r.t to TTransportException. Saying anything about the client side is hard because of all the different language implementations.
However this code is focused on generating a java server, so we can say what the java server will do. The java server will either send TApplicationException or close the connection depending on this setting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100562753
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -91,6 +92,8 @@ class t_java_generator : public t_oop_generator {
reuse_objects_ = true;
} else if( iter->first.compare("option_type") == 0) {
use_option_type_ = true;
+ } else if( iter->first.compare("handle_runtime_exceptions") == 0) {
--- End diff --
I think the behavior of this java generator option be documented. Where should it be documented?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100901172
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -5265,6 +5274,9 @@ THRIFT_REGISTER_GENERATOR(
" android_legacy: Do not use java.io.IOException(throwable) (available for Android 2.3 and "
"above).\n"
" option_type: Wrap optional fields in an Option type.\n"
+ " handle_runtime_exceptions:\n"
+ " Generated services will handle RuntimeException as "
--- End diff --
I think it'd be hard to properly explain the TTransportException the client will experience otherwise, in a short usage text.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100861533
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -5265,6 +5274,9 @@ THRIFT_REGISTER_GENERATOR(
" android_legacy: Do not use java.io.IOException(throwable) (available for Android 2.3 and "
"above).\n"
" option_type: Wrap optional fields in an Option type.\n"
+ " handle_runtime_exceptions:\n"
+ " Generated services will handle RuntimeException as "
--- End diff --
I think the following documentation gives users a little more insight into what this option does. Is there a length limit? I was trying to make the following short.
```
Adjust how server handles RuntimeException (RE). Default is to close connection on RE. This enables sending TApplicationException on RE.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1186: THRIFT-1805 Provide option for handling RTEs
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/thrift/pull/1186
I'm not familiar enough with Thrift's testing framework to add tests, but this works tested locally.
I don't know if this makes sense for other languages, but it seems to make a lot of sense for Java.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1186
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100565597
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -91,6 +92,8 @@ class t_java_generator : public t_oop_generator {
reuse_objects_ = true;
} else if( iter->first.compare("option_type") == 0) {
use_option_type_ = true;
+ } else if( iter->first.compare("handle_runtime_exceptions") == 0) {
--- End diff --
Yep, should be added to the structure at the end of the file at the very least.
It would be nice to have more comprehensive documentation on the java options for the compiler in the java runtime readme, perhaps... but getting it into the compiler's --help output is required as stated above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100873930
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -5265,6 +5274,9 @@ THRIFT_REGISTER_GENERATOR(
" android_legacy: Do not use java.io.IOException(throwable) (available for Android 2.3 and "
"above).\n"
" option_type: Wrap optional fields in an Option type.\n"
+ " handle_runtime_exceptions:\n"
+ " Generated services will handle RuntimeException as "
--- End diff --
The output of `thrift --help` is already quite long, so I don't think an extra line or two is going to matter. What probably matters more is brevity and formatting for readability.
How about simply: `Send TApplicationException to clients when RuntimeException occurs on the server` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1186: THRIFT-1805 Provide option for handling RTEs
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/thrift/pull/1186
@Jens-G I will look and comment on THRIFT-4239.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1186: THRIFT-1805 Provide option for handling RTEs
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1186
@ctubbsii: Could you please have a look at [THRIFT-4239](https://issues.apache.org/jira/browse/THRIFT-4239)? Not sure if it has sth. to do with your patch, but it came up while looking for the cause, so maybe you do have some input.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100917042
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -5265,6 +5274,9 @@ THRIFT_REGISTER_GENERATOR(
" android_legacy: Do not use java.io.IOException(throwable) (available for Android 2.3 and "
"above).\n"
" option_type: Wrap optional fields in an Option type.\n"
+ " handle_runtime_exceptions:\n"
+ " Generated services will handle RuntimeException as "
--- End diff --
Okay, I updated the docs based on my simplified wording above, but also with an explanation of the default behavior.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100586356
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -3527,6 +3531,11 @@ void t_java_generator::generate_process_function(t_service* tservice, t_function
indent(f_service_) << " return " << ((tfunction->is_oneway()) ? "true" : "false") << ";" << endl;
indent(f_service_) << "}" << endl << endl;
+ indent(f_service_) << "@Override" << endl;
+ indent(f_service_) << "protected boolean handleRuntimeExceptions() {" << endl;
+ indent(f_service_) << " return " << ((handle_runtime_exceptions_) ? "true" : "false") << ";" << endl;
+ indent(f_service_) << "}" << endl << endl;
--- End diff --
why not simply `return handle_runtime_exceptions_`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100875982
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -5265,6 +5274,9 @@ THRIFT_REGISTER_GENERATOR(
" android_legacy: Do not use java.io.IOException(throwable) (available for Android 2.3 and "
"above).\n"
" option_type: Wrap optional fields in an Option type.\n"
+ " handle_runtime_exceptions:\n"
+ " Generated services will handle RuntimeException as "
--- End diff --
I like that. I was also trying to document what the default behavior is. When someone is trying to make decision about enabling the option, its nice to know what not enabling it means.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100586606
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -91,6 +92,8 @@ class t_java_generator : public t_oop_generator {
reuse_objects_ = true;
} else if( iter->first.compare("option_type") == 0) {
use_option_type_ = true;
+ } else if( iter->first.compare("handle_runtime_exceptions") == 0) {
--- End diff --
We implemented similar behaviour for other languages, IIRC one was Ruby.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1186: THRIFT-1805 Provide option for handling RTEs
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1186
My guess is there would be fairly easy ways to unit test this behavior so it doesn't need to be an integration test. Also, +1 for using the term Mocket. Love that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1186: THRIFT-1805 Provide option for handling RTEs
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1186#discussion_r100623293
--- Diff: compiler/cpp/src/thrift/generate/t_java_generator.cc ---
@@ -91,6 +92,8 @@ class t_java_generator : public t_oop_generator {
reuse_objects_ = true;
} else if( iter->first.compare("option_type") == 0) {
use_option_type_ = true;
+ } else if( iter->first.compare("handle_runtime_exceptions") == 0) {
--- End diff --
Updated PR with additional commit with --help doc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---