[
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)
