fix Dubbo3.x Context problem #8525 by honganan · Pull Request #110 · apache/skywalking-java

honganan · 2022-02-11T04:02:04Z

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #8525.
Update the CHANGES log.

wu-sheng · 2022-02-11T04:07:10Z

+        return invoker.getUrl()
+                .getParameter("side", "consumer")
+                .equals("provider");


Is this an official way? It seems strange. I am waiting for Dubbo team to response.

We can wait for the Dubbo team...
I write this from my understanding to the scope of Rpc*Context and RpcInvocation.
The RpcServiceContext#isConsumerSide() implements like:

public boolean isConsumerSide() { return this.getUrl().getSide("provider").equals("consumer"); }

If this is already official, could we use RpcServiceContext#isConsumerSide() directly?

The problem is RpcServiceContext not been reset when it's role switched, but RpcInvocation is always correctly bounded to the current request or serve request

Oh, I see. Could you add this as comments on the codes? This is important to keep this in mind why we change now.

OK, same as I see. I will modify it and pass the test.

Yes, we just need to declare the parent class only.

But I can't see the link from your comments, but the conclusion is certain.

#73 (comment)
I reduced 2 Instrumentation to 1 and testing it.

Got it. Let's see.

wu-sheng · 2022-02-20T09:21:42Z

@honganan Could you make this PR works? I am not sure why Dubbocommunity seems not responsing. Let's try at our side.

wu-sheng · 2022-02-23T03:48:42Z

@honganan Any update?

wu-sheng · 2022-02-23T03:49:57Z

-            if (result != null && result.getException() != null) {
-                dealException(result.getException());
-            }


Do we need to process result#getExcetpion?

I haven't received notifications these days, I will debug and make it clear about how it process Exceptions.

Usually, we don't need to catch exceptions, as the handleMethodException method exists already. But we should process result#getExcetpion, because this exception may throw after the whole intercepting is finished or even wouldn't, depending on Dubbo's implementation.

honganan · 2022-02-23T08:59:03Z

I can, in these two days. I need more time to make it clear how Dubbo handle exception.

wu-sheng · 2022-02-24T14:53:07Z

Error:  testConsumerWithException(org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest)  Time elapsed: 2.085 s  <<< ERROR!
java.lang.NullPointerException
	at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithException(DubboInterceptorTest.java:156)

Error:  testConsumerWithResultHasException(org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest)  Time elapsed: 0.013 s  <<< ERROR!
java.lang.NullPointerException
	at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithResultHasException(DubboInterceptorTest.java:169)

Error:  testConsumerWithAttachment(org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest)  Time elapsed: 0.008 s  <<< ERROR!
java.lang.NullPointerException
	at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithAttachment(DubboInterceptorTest.java:144)

@honganan I think you missed some UTs.

wu-sheng

UT fails, pending.

honganan · 2022-02-24T15:09:46Z

I will figure this out tomorrow morning.

…

------------------ Original ------------------ From: 吴晟 Wu Sheng ***@***.***> Date: Thu,Feb 24,2022 10:53 PM To: apache/skywalking-java ***@***.***> Cc: 洪阿南 ***@***.***>, Mention ***@***.***> Subject: Re: [apache/skywalking-java] fix Dubbo3.x Context problem #8525 (PR#110) Error: testConsumerWithException(org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest) Time elapsed: 2.085 s <<< ERROR! java.lang.NullPointerException at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithException(DubboInterceptorTest.java:156) Error: testConsumerWithResultHasException(org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest) Time elapsed: 0.013 s <<< ERROR! java.lang.NullPointerException at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithResultHasException(DubboInterceptorTest.java:169) Error: testConsumerWithAttachment(org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest) Time elapsed: 0.008 s <<< ERROR! java.lang.NullPointerException at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithAttachment(DubboInterceptorTest.java:144) @honganan I think you missed some UTs. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: ***@***.***>

wu-sheng · 2022-02-25T02:52:20Z

I think this mock preparation is not enough, should be enhanced with the URL you expect.

honganan · 2022-02-25T02:54:57Z

I think this mock preparation is not enough, should be enhanced with the URL you expect.

Do you mean add clean side param in the URL in DubboInterceptorTest?

honganan · 2022-02-25T02:58:44Z

It's strange the Unit Test fails with NPE, unless @Before annotated method not invoked. Can you help me check code of DubboInterceptorTest

wu-sheng · 2022-02-25T03:12:17Z

I can't see you mocked the attachment in prepare stage. Only testProviderWithAttachment method has this.

This is easy to reproduce locally.

java.lang.NullPointerException
	at org.apache.skywalking.apm.plugin.asf.dubbo3.DubboInterceptor.beforeMethod(DubboInterceptor.java:99)
	at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithException(DubboInterceptorTest.java:156)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner$1.evaluate(TracingSegmentRunner.java:85)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner$2.call(DelegatingPowerMockRunner.java:149)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner$2.call(DelegatingPowerMockRunner.java:141)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner.withContextClassLoader(DelegatingPowerMockRunner.java:132)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner.run(DelegatingPowerMockRunner.java:141)

honganan · 2022-02-25T03:22:51Z

I can't see you mocked the attachment in prepare stage. Only testProviderWithAttachment method has this.

This is easy to reproduce locally.

java.lang.NullPointerException
	at org.apache.skywalking.apm.plugin.asf.dubbo3.DubboInterceptor.beforeMethod(DubboInterceptor.java:99)
	at org.apache.skywalking.apm.plugin.dubbo3.DubboInterceptorTest.testConsumerWithException(DubboInterceptorTest.java:156)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner$1.evaluate(TracingSegmentRunner.java:85)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner$2.call(DelegatingPowerMockRunner.java:149)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner$2.call(DelegatingPowerMockRunner.java:141)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner.withContextClassLoader(DelegatingPowerMockRunner.java:132)
	at org.powermock.modules.junit4.internal.impl.DelegatingPowerMockRunner.run(DelegatingPowerMockRunner.java:141)

I found the problem, as changed isProvider() name and the default value last night and did not update code today.
I will add 2 mock URL separately for Consumer and Provider.

AlbumenJ · 2022-03-07T03:10:46Z

-        } else {
-            isConsumer = serviceContext.isConsumerSide();
-        }
+        boolean isConsumer = isConsumer(invocation);


@wu-sheng I'm a little confused about this. Why not judge whether it is the consumer side directly from EnhancedInstance objInst. If objInst is a MonitorFilter object, it should be on the provider side. If objInst is a MonitorClusterFilter object, it should be on the provider side.

We just enhanced MonitorFilter class finally, as MonitorClusterFilter extends MonitorFilter and enhance both of them will cause problem. We have talked above.

@AlbumenJ Agree with @honganan , as a byte code manipulator, once we change the parent class, and the sub classes don't override the changed method, this enhancement actually changes the behavior of all classes on this hierarchy tree.

@wu-sheng got it!

honganan added 2 commits February 11, 2022 11:53

fix Dubbo3.x Context problem #8525

93c783e

CHANGES.md

041ed5c

wu-sheng reviewed Feb 11, 2022

View reviewed changes

honganan added 2 commits February 11, 2022 13:54

refactor import and add licences

c555104

refactor import

c0d1d54

wu-sheng added plugin enhancement New feature or request labels Feb 11, 2022

honganan and others added 4 commits February 11, 2022 17:20

modify test data

d6b7544

Merge branch 'main' into dubbo3_context_fix

3f2044b

fix log error twice problem

a703ac4

refactor imports

6c1f680

Merge branch 'main' into dubbo3_context_fix

bcd4dc7

wu-sheng reviewed Feb 23, 2022

View reviewed changes

fix enhance twice causing of class inherit problem

805be6a

wu-sheng reviewed Feb 24, 2022

View reviewed changes

Comment thread ...o-3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/asf/dubbo3/DubboInterceptor.java Outdated

wu-sheng mentioned this pull request Feb 24, 2022

Potential RpcServiceContext contamination in Dubbo 3 apache/dubbo#9666

Closed

1 task

refactor method name

10f45e8

wu-sheng approved these changes Feb 24, 2022

View reviewed changes

wu-sheng requested changes Feb 24, 2022

View reviewed changes

enhance unit test scenario

0b9a1ab

wu-sheng added this to the 8.10.0 milestone Feb 25, 2022

wu-sheng approved these changes Feb 25, 2022

View reviewed changes

wu-sheng merged commit 9efec16 into apache:main Feb 25, 2022

AlbumenJ reviewed Mar 7, 2022

View reviewed changes

Conversation

honganan commented Feb 11, 2022 • edited by wu-sheng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Feb 20, 2022

Uh oh!

wu-sheng commented Feb 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honganan commented Feb 23, 2022

Uh oh!

Uh oh!

wu-sheng commented Feb 24, 2022

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

honganan commented Feb 24, 2022 via email

Uh oh!

wu-sheng commented Feb 25, 2022

Uh oh!

honganan commented Feb 25, 2022

Uh oh!

honganan commented Feb 25, 2022

Uh oh!

wu-sheng commented Feb 25, 2022

Uh oh!

honganan commented Feb 25, 2022

Uh oh!

AlbumenJ Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

honganan commented Feb 11, 2022 •

edited by wu-sheng

Loading

AlbumenJ Mar 7, 2022 •

edited

Loading