[jira] [Comment Edited] (THRIFT-1805) Thrift should not swallow ALL exceptions

["Jens Geyer (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-1805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15144629#comment-15144629 ] 

Jens Geyer edited comment on THRIFT-1805 at 2/12/16 2:33 PM:
-------------------------------------------------------------

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 or unexpectedly drop the connection. 

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 crashes 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?




was (Author: jensg):
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)