[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16072796#comment-16072796 ] ASF GitHub Bot commented on THRIFT-1805: Github user ctubbsii commented on the issue: https://github.com/apache/thrift/pull/1186 @Jens-G I will look and comment on THRIFT-4239. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Christopher Tubbs > Fix For: 0.11.0 > > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16071299#comment-16071299 ] ASF GitHub Bot commented on THRIFT-1805: 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Fix For: 0.11.0 > > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15887241#comment-15887241 ] ASF GitHub Bot commented on THRIFT-1805: Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1186 > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15879717#comment-15879717 ] ASF GitHub Bot commented on THRIFT-1805: 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15879666#comment-15879666 ] Christopher Tubbs commented on THRIFT-1805: --- In Accumulo, I added a test, "ThriftBehaviorIT.java" which uses a bi-directional in-memory transport to test exception handling behavior in Thrift, using our internal "RpcWrapper" to wrap the service processor and catch RuntimeExceptions. (contained in this commit: https://github.com/apache/accumulo/commit/86e6fb44bbf59109a61b159d2bd3bcd0fcfdfa9f) I'd like the test (or something like it) to be adapted to prevent regressions in Thrift directly. I might even be willing to work on it. However, the current behavior is undefined when the server encounters a RuntimeException, and undefined behavior is untestable. I'm not sure a regression test even makes sense unless the most recent regression is also reverted back to a well-defined behavior. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15877115#comment-15877115 ] James E. King, III commented on THRIFT-1805: [~jensg], [~nsuke], [~roger.meier] could one (or more, of course!) of you review the current pull request which completed CI testing successfully? There was a lot of discussion on this and I don't want to merge it until at least one of you has a chance to review and approve. https://github.com/apache/thrift/pull/1186 > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864602#comment-15864602 ] ASF GitHub Bot commented on THRIFT-1805: 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864547#comment-15864547 ] ASF GitHub Bot commented on THRIFT-1805: Github user keith-turner commented on a diff in the pull request: https://github.com/apache/thrift/pull/1186#discussion_r10097 --- 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864504#comment-15864504 ] James E. King, III commented on THRIFT-1805: [~jensg], [~nsuke], [~roger.meier] given you had significant input to this one, you should consider reviewing the solution and approve the pull request on github if you would like it to merge. There have been two approvals already. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864471#comment-15864471 ] ASF GitHub Bot commented on THRIFT-1805: 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864270#comment-15864270 ] ASF GitHub Bot commented on THRIFT-1805: 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864250#comment-15864250 ] ASF GitHub Bot commented on THRIFT-1805: 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` ? > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15864163#comment-15864163 ] ASF GitHub Bot commented on THRIFT-1805: 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. ``` > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15861847#comment-15861847
]
ASF GitHub Bot commented on THRIFT-1805:
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.
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15861593#comment-15861593 ] Jens Geyer commented on THRIFT-1805: > why not simply `return handle_runtime_exceptions_`? Because it would be wrong. ;-) > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15861586#comment-15861586
]
ASF GitHub Bot commented on THRIFT-1805:
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.
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15861584#comment-15861584
]
ASF GitHub Bot commented on THRIFT-1805:
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_`?
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15861460#comment-15861460
]
ASF GitHub Bot commented on THRIFT-1805:
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.
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15861445#comment-15861445
]
ASF GitHub Bot commented on THRIFT-1805:
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?
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15860657#comment-15860657 ] ASF GitHub Bot commented on THRIFT-1805: 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. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15860648#comment-15860648 ] ASF GitHub Bot commented on THRIFT-1805: 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 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 > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15146005#comment-15146005 ] Jens Geyer commented on THRIFT-1805: I said very intentionally "*last* line of defense" and "catch *uncaught* errors". There is no need to handle things again that are already properly handled. Bottom line => +1 > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15145837#comment-15145837 ] Aki Sukegawa commented on THRIFT-1805: -- [~jensg] sorry that was indeed vague. "current" -> current Java behavior that only catches TException "default' -> sending back TApplicationException generally, and potentially what is described in THRIFT-3607 So we're basically talking about the same. Slightly difference is about TTransportException caused by disconnect where the processor have no means to send an exception back any more and rather needs to quit promptly. Many TProcessor implementations try to send errors through broken transport **outside** try blocks in such occasions. It naturally results in another TTransportException from the now-broken transport, but typically it does not kill the server but only the processor object/thread. Some implementations specifically rethrow TTransportException and only catch the other errors. I found the latter preferrable to the former and it's where my view is slightly different from your view on "last line of defense": here, I'm not seeking to ensure catch-all behavior, based on an observation that current servers are surviving TTransportExceptions just fine. Thinking about it, the current proposal at least needs to additionally allow "catch TTransportException and gracefully exit" behavior, as it can be even better than explicit rethrow. You may still prefer making it TProcessor's clear responsibility to handle all exceptions, though. So far, I'll change my proposal to only specify mappings from "handler errors" to "observable behaviors from client", leaving it to the implementation how to do that, i.e.: * TApplicationException -> see the message and the type as thrown, not hidden by INTERNAL_ERROR * TTransportException -> connection is either already broken or newly broken * other -> TApplicationException(INTERNAL_ERROR) Further feedback is very much appreciated, from Jens or anyone else. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15144629#comment-15144629
]
Jens Geyer commented on THRIFT-1805:
Not sure if we really talk about the same. I am also not only talking about
Java, so I have a slight problem with what could be the "current behaviour".
What I mean is the "last line of defense", if you will. There is no doubt that
in an ideal world any exception would be properly caught, but, unfortunately,
in real life things happen. I talk about the additional code in the processor
implementation that ensures that any uncaught exception does not blow up the
server.
First, if some call is able to achieve this, even by accident, this is the way
to the next "Limit recursion depth to 64" issue. Second, dropping a connection
because the server (not the client!) has an implementation bug and thus throws
under some otherwise legitimate circumstances, is somewhat unexpected to the
well-behaving client.
Anyone is free to catch exceptions at any place and add whatever code is needed
to keep "statistics on which client causes errors" (again, it doesn't have to
be the client where the problem is). But **that's really an addon
functionality** and if I don't install such an feature, I still don't want
anybody to be able to DOS my server simply by calling a buggy piece of
implementation.
{quote}
we can remove the current behavior but be open to alternative suggestions that
does not modify the default behavior.{quote}
Just for the records: What is **current** and what is **default** behaviour?
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15143766#comment-15143766 ] Aki Sukegawa commented on THRIFT-1805: -- Should have noted here: I've proposed a policy for TProcessor exception handling in THRIFT-3607 . If everyone is happy with it, the plan is to update cross language tests, after that we have a consensus on Java sync processor here. The current behavior does give more control to users if (and only if) they're implementing custom servers. To me, this extra control should be added on top of "standard" behavior if at all, that is to send TApplicationException back. An example is TTransport::getOrigin method in C++ that gets clients' address. It opens a possibility for users' applications to, e.g., accumulate statistics on which client causes errors and prohibit them using external proxy etc. So the bottom line is that, in my opinion, we can remove the current behavior but be open to alternative suggestions that does not modify the default behavior. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15143491#comment-15143491 ] Christopher Tubbs commented on THRIFT-1805: --- The original design (since version 0.2, THRIFT-378) did seem to provide a useful exception back to the clients (TApplicationException), which was clearly distinguishable from intended exceptions sent from the server created by the user, and also easily distinguishable from client-side exceptions due to, say, temporary network errors. I really liked how that original behavior worked, but it has flip-flopped with THRIFT-447 in 0.7, THRIFT-1658 in 0.9, THRIFT-1805 in 0.9.1. I'm not sure the framework should catch server-side Errors (those can't really be handled reliably, but an argument could be made for a best-effort attempt to let the client know anyway)... but server-side RuntimeExceptions, yeah, that'd be very helpful helpful for the framework to turn into TApplicationExceptions like it was intended to do since 0.2/THRIFT-378. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15143433#comment-15143433 ] Jens Geyer commented on THRIFT-1805: Of course, just swallowing and telling clients "all is rosy over here" is not an option. There's no doubt about that. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15143431#comment-15143431
]
Jens Geyer commented on THRIFT-1805:
{quote}
It seems that other languages including C++, Go, C#, Node.js, Ruby and Haskell
catche all errors and send exceptions back to the client.
{quote}
There has been some discussion around this but in my humble personal opinion
this should be the default. I don't want anybody to crash my server or to
hard-drop the connection just because some edge case in some arbitrary routine
is not properly handled (which in real life happens) and throws.
I also can't see the value in being required otherwise to add that very same
code to every single interface method implementation by hand if I don't want
the default crashy behaviour. That's why I use a framework and a code generator
in the first place, isn't it? And as it turned out, some other people seem to
have the same view of things.
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Diwaker Gupta
> Attachments: THRIFT-1805.patch
>
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15140585#comment-15140585 ] Aki Sukegawa commented on THRIFT-1805: -- It seems that other languages including C++, Go, C#, Node.js, Ruby and Haskell catche all errors and send exceptions back to the client. I think the default behavior at least should be the same as other implementations. If we want to preserve the current behavior, an option may be added to TProcessor and its derived types, for example. BTW, Java async processor is also problematic. It truly swallows everything and does not even bother disconnecting. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15049363#comment-15049363 ] Roger Meier commented on THRIFT-1805: - [~elserj], you are right. It is up to you to show interest and contribute on the Java part of Thrift. There are not that many patches and improvement coming in for Java nowadays. Managing every api change within PMC is not ideal as we do not have enough Java stakeholders there. We need to discuss these topics within Jira or the dev mailing list to get some more Java people involved. There are many other projects out there using Thrift without the Apache sister projects and they use other languages such as go, nodejs, c++. I would really like to have more involvement of Apache sister projects and I guess most of them depend on Java and they should integrate very smooth. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15047459#comment-15047459 ] Christopher Tubbs commented on THRIFT-1805: --- Sure, I definitely think a regular contributor could carry this torch, but I just mean that some of the decision-making needs to be done by the PMC. Regular contributors can ask for Semantic Versioning, but cannot establish that as a policy for the project. That's the job of the PMC. Thrift has been very welcoming of contributions, but that can also be part of the problem: contributions which are mutually exclusive are both welcomed and accepted, and behavior flip-flops between versions. At some point, the PMC needs to make a decision about which behavior is the desired behavior, in order to guide future contributions. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15047410#comment-15047410 ] Josh Elser commented on THRIFT-1805: Roger's comment really hits the nail on the head: bq. You are welcome to help improving Java within Thrift [~ctubbsii], I think all of the things you suggested could still be driven by someone carrying the torch. It's not like there are examples where Thrift knowingly denied things that would have helped the points you outlined. It's a priority to you (us, depending on the hat I'm wearing). If we want to see action, maybe we are the ones who need to push for the good changes you outlined. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15047344#comment-15047344 ] Christopher Tubbs commented on THRIFT-1805: --- I'm not sure it's a matter of just improving through patches/testing/QA fixes. I think it's also a matter of PMC direction to determine some guidelines on behavior for the code, so it doesn't regress every time somebody has a new use case. Patches can be evaluated against those guidelines. In this case, for example, there was a regression introduced, which had occurred and been fixed before (THRIFT-1658). Examining the code, there appears to have been a conscious decision at one point to create two classes of exceptions, yet the regression overruled this prior decision. That shouldn't have happened if that decision had been encapsulated into some guidelines for expected behavior. In other cases, regressions have flip-flopped (THRIFT-2173 and related) several times, each time the patch being accepted without much question, even though it reversed a prior decision. In addition to patches/testing/QA, I think the Thrift PMC could help this situation by making some decisions for the project going forward, to increase stability. Some such things: 1. Adopt Semantic Versioning with a well-defined public API which addresses libraries, the IDL, and generated code. 2. Adopt a policy for maintaining on-the-wire compatibility in terms of the versioning scheme chosen. 3. When a change in behavior causes problems and a decision must be made, document the behavior which was decided upon (even if it's just a comment in the code), so that future patches do not reverse prior decisions, without some degree of oversight. These aren't foolproof, but some guidelines for the direction of the project could help contributors make higher quality contributions. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15043579#comment-15043579 ] Roger Meier commented on THRIFT-1805: - You are welcome to help improving Java within Thrift, patches or verification of available patches are very welcome. Also overall code quality fixes for the Java code base. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15036776#comment-15036776 ] Christopher Tubbs commented on THRIFT-1805: --- Over a year later and this issue is still affecting us. I think this was quite an annoying regression. TApplicationException served an important role on the client side... similar to an HTTP 5xx error vs. other HTTP errors. Collapsing them, as this seems to do, makes workarounds to recreate that behavior pretty cumbersome. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15033849#comment-15033849 ] Keith Turner commented on THRIFT-1805: -- Just want to chime in and explain Accumulo's use case. Accumulo relied on TApplicationException in the following way. * When Accumulo client code saw a TApplicationException it assumed an unexpected server side error had occurred and propogated this to the user by throwing an AccumuloServerException. * When Accumulo client code saw a TTransportException, it assumed some sort of network problem occurred or a tablet server died. In this case Accumulo client code will automatically retry w/o the user ever being aware. This change in behavior was very disruptive for Accumulo. We worked around it, but just ran into an issue with our work around for the one-way method case. See ACCUMULO-4065 if interested and a [proposed work around|https://github.com/apache/accumulo/pull/56]. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14043430#comment-14043430 ] Ben Craig commented on THRIFT-1805: --- Adding an exception to a class doesn't break the wire format, but it probably doesn't do what you want either. Exceptions in the .thrift file are treated similarly to optional members of a struct. It will get serialized across, but if the receiver isn't expecting it, the receiver will discard it. As to Christopher's problem, I can see reasonable arguments for either behavior (close socket on error vs. send TApplicationException on error). My general preference is to close the socket on errors, but maybe it would make sense to provide a constructor arg or codegen flag to let people choose. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.2#6252)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14042803#comment-14042803 ] Christopher Tubbs commented on THRIFT-1805: --- After some further investigation, it looks like the problem is worse than it looked. This patch left no mechanism to even get an application's TException to the ProcessFunction class where this would be wrapped. The only way it looks to preserve the behavior of clients being aware of server-side errors is to write a separate user exception class, to be serialized on the wire, which breaks the wire compatibility between 0.9.0 and 0.9.1 (as I understand it). > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.2#6252)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14042756#comment-14042756 ] Christopher Tubbs commented on THRIFT-1805: --- Contrary to the description and intent of this issue, it seems that this breaks things for users expecting consistent behavior with 0.9.0 (and 0.6.0 and earlier), and actually causes thrift to hide exception information from the client. The expected behavior was that unknown server-side errors would result in a TApplicationException.INTERNAL_ERROR, but now it just closes the connection and the client sees a network exception (TTransportException) instead, which a reasonable client might just conclude was a temporary network hiccup and retry. > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message was sent by Atlassian JIRA (v6.2#6252)
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13561073#comment-13561073 ] Hudson commented on THRIFT-1805: Integrated in Thrift #619 (See [https://builds.apache.org/job/Thrift/619/]) THRIFT-1805 Thrift should not swallow ALL exceptions (Revision 7b96b2249a43ae75b48e0aba7e8beffc67b32d93) Result = SUCCESS roger : https://git-wip-us.apache.org/repos/asf?p=thrift.git&a=commit&h=7b96b2249a43ae75b48e0aba7e8beffc67b32d93 Files : * lib/java/src/org/apache/thrift/ProcessFunction.java > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Diwaker Gupta > Attachments: THRIFT-1805.patch > > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[
https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13559155#comment-13559155
]
Diwaker Gupta commented on THRIFT-1805:
---
[~roger.meier], I was able to run the Java unit tests successfully without any
problems. I don't see why my suggested change should have any impact on
ServerTestBase.testException. I'm now trying to run the integration test but
they have other issues on OS X (no {{timeout}} command, for instance) because
of which I'm unable to verify.
> Thrift should not swallow ALL exceptions
>
>
> Key: THRIFT-1805
> URL: https://issues.apache.org/jira/browse/THRIFT-1805
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
>Affects Versions: 0.9
>Reporter: Diwaker Gupta
>Assignee: Jake Farrell
>
> In Thrift 0.8.0, Thrift generated Java code did not swallow application
> exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and
> now the generated code swallows ALL application exceptions (via
> ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while
> I see the rationale, it is breaking our applications.
> Our code relies on the fact that exceptions can propagate outside of Thrift
> for certain things (e.g., to aggressively drop connections for clients that
> send invalid/malformed requests). ProcessFunction makes it near impossible to
> do this -- not only does it swallow the exception, it also loses all
> information about the original exception and just writes out a generic
> TApplicationException.
> IMO ProcessFunction should only catch TException. If the application code
> wants to use other exceptions for some reason (in particular, Errors and
> RuntimeExceptions), Thrift shouldn't prevent that.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1805) Thrift should not swallow ALL exceptions
[ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13545493#comment-13545493 ] Roger Meier commented on THRIFT-1805: - Only catch TException via ProcessFunction makes sense. Java has several issues when I run the cross language test suite(THRIFT-847) see ThriftTest.thrift and test/test.sh could you have a look at the existing testException Method and provide a patch for this as well? https://git-wip-us.apache.org/repos/asf?p=thrift.git;a=blob;f=lib/java/test/org/apache/thrift/server/ServerTestBase.java; > Thrift should not swallow ALL exceptions > > > Key: THRIFT-1805 > URL: https://issues.apache.org/jira/browse/THRIFT-1805 > Project: Thrift > Issue Type: Bug > Components: Java - Compiler, Java - Library >Affects Versions: 0.9 >Reporter: Diwaker Gupta >Assignee: Jake Farrell > > In Thrift 0.8.0, Thrift generated Java code did not swallow application > exceptions. As a result of THRIFT-1658, this behavior changed in 0.9.0 and > now the generated code swallows ALL application exceptions (via > ProcessFunction). Apparently this was the behavior in Thrift 0.6.0 and while > I see the rationale, it is breaking our applications. > Our code relies on the fact that exceptions can propagate outside of Thrift > for certain things (e.g., to aggressively drop connections for clients that > send invalid/malformed requests). ProcessFunction makes it near impossible to > do this -- not only does it swallow the exception, it also loses all > information about the original exception and just writes out a generic > TApplicationException. > IMO ProcessFunction should only catch TException. If the application code > wants to use other exceptions for some reason (in particular, Errors and > RuntimeExceptions), Thrift shouldn't prevent that. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira
