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

Reply via email to