LoggingThriftMethodInterceptor exception wrapping

[jihor]

Hi! I see that LoggingThriftMethodInterceptor wraps any exception into a TApplicationException in afterThrowing(), TApplicationException being a checked exception. I did a small refactoring of that class and have just left this code as it was. But now it's bothering me. If my callee code results in some runtime exception, the exception chain gets ugly: SomeRuntimeException -> TApplicationException -> java.lang.reflect.UndeclaredThrowableException: null (!) -> org.apache.thrift.transport.TTransportException: HTTP Response code: 406

Maybe it's worth changing the exception type to some runtime exception subclass? The problem is, all libthrift exceptions are checked.

[aatarasoff]

Hi, could you provide real stacktraces on client side and server side?

[jihor]

Thrift generated sources always declare a org.apache.thrift.TException on methods:

public class TService {
    public interface Iface {
        public Response act() throws MyException, org.apache.thrift.TException;
        ...
    }
}

An implementing class is usually overriding the method with declaring only a custom exception. Since it never throws a org.apache.thrift.TException, why bother declaring it? Example:

public class ThriftEndpoint implements TService.Iface
@Override
public Response act() throws MyException {...}

In runtime, methods of the implementing class are proxied by LoggingThriftMethodInterceptor, and should a subclass of RuntimeException be thrown from the method, the interceptor wil wrap it into a TApplicationException and rethrow it. Since TApplicationException is not declared in method signature of the implementing class, it becomes an UndeclaredThrowableException and then a TTransportException with message HTTP Response code: 406.

Possible workarounds:

1. always declare org.apache.thrift.TException on methods in implementing classes, even if they are not actually thrown by the method code itself:

@Override
Response act() throws MyException, org.apache.thrift.TException {...}

This will lead to TTransportException with Internal error processing {methodName} message, but message from the original exception will not be delivered to client, so this workaround is obviosly not ideal.

2. catch any Exceptions in method and wrap them into declared custom exception (it always extends TException and will not be wrapped by interceptor):

@Override
Response act() throws MyException {
    try {
        ...
    } catch (Exception e) {
        throw new MyException(e);
    }
}

I've provided logs here: logs.zip Also I've written some tests in my fork (https://github.com/jihor/spring-thrift-starter), see TGreetingServiceHandlerTests class, methods are:

• testMappedClientWithCustomException() - corresponds to workaround (2);
• testMappedClientWithRuntimeException1() - corresponds to workaround (1);
• testMappedClientWithRuntimeException2() - this is what you get by default, an HTTP Response code: 406;

It might be useful to merge these tests into master.

[jihor]

Hmm, maybe it's worth deleting any exception wrapping from LoggingThriftMethodInterceptor. It's an interceptor for logging an it shouldn't interfere with application flow. Then, should you throw a RuntimeExcepton from your method, you will get a TTransportException on client regardless of the method signature.

EDIT: You will actually still get HTTP Response code: 406, but this time it will not depend on method signature. Well, still better than getting different exception classes based on what did you define on your method.