Provide consumers with real exception thrown by providers #3387

eachcan · 2019-01-30T02:21:29Z

What is the purpose of the change

Fix #3386

Brief changelog

Consumer can catch the same exception as which thrown by provider.

Verifying this change

Verified

Follow this checklist to help us incorporate your contribution quickly and easily:

...pache/dubbo/config/spring/beans/factory/annotation/ReferenceAnnotationBeanPostProcessor.java

CrazyHZM · 2019-01-30T02:31:26Z

...pache/dubbo/config/spring/beans/factory/annotation/ReferenceAnnotationBeanPostProcessor.java

+                    e = t;
+                }
+
+                throw e;


It can be thrown directly here, it does not need to judge, what do you think?

接口声明中 throws 的异常是 ValidationException，但是消费者捕获到的却是 UndeclaredThrowableException，此处通过解析出最内层的 Throwable，来符合接口声明。

为何最内层的 Throwable 可以符合接口声明呢？

在我们使用了 ExceptionFilter 的情况下，将会由 ExceptionFilter 构造出同 Provider 中抛出的相同的 Exception ({"error": { "data": {"name": "...."}}})，此时可以捕获到相同的异常（Stacktrace不相同，此处暂时忽略，需要其他解决方案）。

在没有使用 ExceptionFilter 的情况下，将会由 com.googlecode.jsonrpc4j.DefaultExceptionResolver 抛出一个可以由消费者使用的异常

以上两种情况抛出的异常均是消费者感兴趣的，但是由于通过 Proxy 调用 invoker，invoker 抛出的所有异常均会包装为 UndeclaredThrowableException，故我们需要在 handler 中将此异常还原为 invoker 中的异常。

Hi, @CrazyHZM.

Will this pr be merge into master branch? Or else be refused?

English version of previous answer:

Why does it need to judge?

Service interface defines throws ValidationException, but try { ... } catch (ValidationException e) { } in consumer side do not work.

Consumer side gets UndeclaredThrowableException instead. So I add this pr to convert this Exception to the expected one.

How to do this

DefaultExceptionResolver or ExceptionFilter will repackage the exception from provider side, so we go inside and re-throw the innermost one.

This works.

Looks good to me.
@beiwei30
Please you review this PR.

codecov-io · 2019-01-30T02:41:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@19c1af8). Click here to learn what that means.
The diff coverage is 28.57%.

@@            Coverage Diff            @@
##             master    #3387   +/-   ##
=========================================
  Coverage          ?   63.85%           
  Complexity        ?       71           
=========================================
  Files             ?      661           
  Lines             ?    28608           
  Branches          ?     4822           
=========================================
  Hits              ?    18269           
  Misses            ?     8116           
  Partials          ?     2223

Impacted Files	Coverage Δ	Complexity Δ
...notation/ReferenceAnnotationBeanPostProcessor.java	`88.31% <28.57%> (ø)`	`0 <0> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19c1af8...2f402dc. Read the comment docs.

tswstarplanet

I think typically, RPC interfaces don’t declare a exception to throw. So if the developer doesn’t declare a checked exception, the modification may eat some runtime exception stacktrace information.

tswstarplanet · 2019-01-31T02:19:14Z

I think giving the entire exception stacktrace to application layer can help developer trace problems better.

beiwei30

I don't think this is the right approach.

beiwei30 · 2019-01-31T08:10:37Z

I write a simple demo to mimic this problem but fail to reproduce, see the output below:

java.lang.RuntimeException: hello world
	at org.apache.dubbo.samples.basic.impl.DemoServiceImpl.sayHello(DemoServiceImpl.java:27)
	at com.alibaba.dubbo.common.bytecode.Wrapper1.invokeMethod(Wrapper1.java)
	at com.alibaba.dubbo.rpc.proxy.javassist.JavassistProxyFactory$1.doInvoke(JavassistProxyFactory.java:47)
	at com.alibaba.dubbo.rpc.proxy.AbstractProxyInvoker.invoke(AbstractProxyInvoker.java:76)
	at com.alibaba.dubbo.config.invoker.DelegateProviderMetaDataInvoker.invoke(DelegateProviderMetaDataInvoker.java:52)
	at com.alibaba.dubbo.rpc.protocol.InvokerWrapper.invoke(InvokerWrapper.java:56)
	at com.alibaba.dubbo.rpc.filter.ExceptionFilter.invoke(ExceptionFilter.java:62)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.monitor.support.MonitorFilter.invoke(MonitorFilter.java:75)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.filter.TimeoutFilter.invoke(TimeoutFilter.java:42)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.protocol.dubbo.filter.TraceFilter.invoke(TraceFilter.java:78)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.filter.ContextFilter.invoke(ContextFilter.java:73)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.filter.GenericFilter.invoke(GenericFilter.java:138)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.filter.ClassLoaderFilter.invoke(ClassLoaderFilter.java:38)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.filter.EchoFilter.invoke(EchoFilter.java:38)
	at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper$1.invoke(ProtocolFilterWrapper.java:72)
	at com.alibaba.dubbo.rpc.protocol.dubbo.DubboProtocol$1.reply(DubboProtocol.java:104)
	at com.alibaba.dubbo.remoting.exchange.support.header.HeaderExchangeHandler.handleRequest(HeaderExchangeHandler.java:96)
	at com.alibaba.dubbo.remoting.exchange.support.header.HeaderExchangeHandler.received(HeaderExchangeHandler.java:173)
	at com.alibaba.dubbo.remoting.transport.DecodeHandler.received(DecodeHandler.java:51)
	at com.alibaba.dubbo.remoting.transport.dispatcher.ChannelEventRunnable.run(ChannelEventRunnable.java:57)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

I am wondering your mailService is enhanced by Spring framework. You could debug your app and double check.

khanimteyaz · 2019-02-03T17:53:47Z

...pache/dubbo/config/spring/beans/factory/annotation/ReferenceAnnotationBeanPostProcessor.java

+            try {
+                return method.invoke(bean, args);
+            } catch (Throwable e) {
+                logger.error("Convert RpcException to real exception");


i think better to log throwable also e.g. logger.error("Convert RpcException to real exception",t);

eachcan · 2019-02-13T10:52:53Z

@beiwei30

I write a simple demo to mimic this problem but fail to reproduce, see the output below:
...

The exception you got was logged in provider side, and the problem I wanted to describe is occurred in consumer side.

I want to:

make consumer side get the same exception with the one thrown by provider

NOTICE: ONLY with same name, NOT contains call stacks

Thanks to @tswstarplanet for pointing out the weakness of this superficial resolution. More effective code is recommended:

        @Override
        public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
            try {
                return method.invoke(bean, args);
            } catch (UndeclaredThrowableException e) {
                logger.error("Convert RpcException to real exception");
                // Only care InvocationTargetException in UndeclaredThrowableException
                if (e.getCause() instanceof InvocationTargetException) {
                    throw e.getCause().getCause();
                } else {
                    throw e;
                }
            }
        }

…ovider**

eachcan · 2019-02-13T11:30:30Z

@beiwei30 new code is ready. Review please.

CLAassistant · 2019-09-06T10:27:21Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

zhangjinshuai seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

AlbumenJ · 2021-02-23T15:30:01Z

Currently, ReferenceAnnotationBeanPostProcessor has been reformatted for serval times.

Related source code is no longer existed in the latest version of master branch.

Please feel free to reopen this pr if you still have any question, also you can create a new pr for related code.

Provide consumers with real exception thrown by providers

2f402dc

CrazyHZM reviewed Jan 30, 2019

View reviewed changes

...pache/dubbo/config/spring/beans/factory/annotation/ReferenceAnnotationBeanPostProcessor.java Outdated Show resolved Hide resolved

CrazyHZM reviewed Jan 30, 2019

View reviewed changes

tswstarplanet reviewed Jan 31, 2019

View reviewed changes

beiwei30 requested changes Jan 31, 2019

View reviewed changes

khanimteyaz reviewed Feb 3, 2019

View reviewed changes

Merge branch 'master' of git://github.com/apache/incubator-dubbo

16990ec

zhangjinshuai added 2 commits February 13, 2019 19:08

**make consumer side get the same exception with the one thrown by pr…

172809e

…ovider**

discard the '.*' form imports

42042a2

AlbumenJ closed this Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide consumers with real exception thrown by providers #3387

Provide consumers with real exception thrown by providers #3387

eachcan commented Jan 30, 2019

CrazyHZM Jan 30, 2019

eachcan Jan 30, 2019

eachcan Jan 30, 2019

eachcan Jan 30, 2019

CrazyHZM Jan 30, 2019

codecov-io commented Jan 30, 2019

tswstarplanet left a comment

tswstarplanet commented Jan 31, 2019

beiwei30 left a comment

beiwei30 commented Jan 31, 2019 •

edited

khanimteyaz Feb 3, 2019 •

edited

eachcan commented Feb 13, 2019 •

edited

eachcan commented Feb 13, 2019

CLAassistant commented Sep 6, 2019

AlbumenJ commented Feb 23, 2021

Provide consumers with real exception thrown by providers #3387

Provide consumers with real exception thrown by providers #3387

Conversation

eachcan commented Jan 30, 2019

What is the purpose of the change

Brief changelog

Verifying this change

CrazyHZM Jan 30, 2019

Choose a reason for hiding this comment

eachcan Jan 30, 2019

Choose a reason for hiding this comment

eachcan Jan 30, 2019

Choose a reason for hiding this comment

eachcan Jan 30, 2019

Choose a reason for hiding this comment

CrazyHZM Jan 30, 2019

Choose a reason for hiding this comment

codecov-io commented Jan 30, 2019

Codecov Report

tswstarplanet left a comment

Choose a reason for hiding this comment

tswstarplanet commented Jan 31, 2019

beiwei30 left a comment

Choose a reason for hiding this comment

beiwei30 commented Jan 31, 2019 • edited

khanimteyaz Feb 3, 2019 • edited

Choose a reason for hiding this comment

eachcan commented Feb 13, 2019 • edited

eachcan commented Feb 13, 2019

CLAassistant commented Sep 6, 2019

AlbumenJ commented Feb 23, 2021

beiwei30 commented Jan 31, 2019 •

edited

khanimteyaz Feb 3, 2019 •

edited

eachcan commented Feb 13, 2019 •

edited