Fix the problem that the consumer stub event does not trigger #9723
Conversation
| received(channel, invocation); | ||
| } catch (Throwable t) { | ||
| logger.warn("Failed to invoke event method " + invocation.getMethodName() + "(), cause: " + t.getMessage(), t); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void getOrWaitStubService(Channel channel, Invocation invocation) throws InterruptedException { | ||
| try { | ||
| Invoker<?> invoker = getInvoker(channel, invocation); |
There was a problem hiding this comment.
If getInvoker throws an exception, it means that the stub service has not been exposed, it will try to wait for the signal that the stub exposure is complete, and then call back the related methods of the stub service.
| @@ -253,7 +276,8 @@ private boolean isClientSide(Channel channel) { | |||
| // if it's callback service on client side | |||
| isStubServiceInvoke = Boolean.TRUE.toString().equals(inv.getObjectAttachments().get(STUB_EVENT_KEY)); | |||
| if (isStubServiceInvoke) { | |||
| port = channel.getRemoteAddress().getPort(); | |||
| // The port of the client exposed stub service is 0 | |||
| port = 0; | |||
There was a problem hiding this comment.
When the consumer exposes the stub service, the port number is 0
There was a problem hiding this comment.
One similar PR has been merged into the master branch
Codecov Report
@@ Coverage Diff @@
## 3.0 #9723 +/- ##
============================================
+ Coverage 65.60% 65.79% +0.19%
- Complexity 295 320 +25
============================================
Files 1206 1212 +6
Lines 52323 52810 +487
Branches 7942 8008 +66
============================================
+ Hits 34325 34747 +422
- Misses 14259 14288 +29
- Partials 3739 3775 +36
Continue to review full report at Codecov.
|
…_event_does_not_trigger
| @@ -702,6 +728,8 @@ public void destroy() { | |||
| } | |||
| } | |||
| referenceClientMap.clear(); | |||
|
|
|||
| stubServiceToLatch.clear(); | |||
There was a problem hiding this comment.
I think we should take destroy of stubServiceToLatch on destroy of stub proxy too.
There might have lots of proxy invokers (reference config) create and destroy during the lifetime of DubboProtocol.
There was a problem hiding this comment.
I don't think waiting for the stub to be ready is necessary for it will make the whole process more complicated. Just giving a more elegant and clear Exception or result would be enough. What do you think? @BurningCN
There was a problem hiding this comment.
Consider the complexities of latch.await(), Can stubServiceToLatch be replaced with this similar structure, Map<String,Boolean> stubService2Flag.When the connect event occurs and the stub invoker is not found, execute stubService2Flag.put(serviceKey, true).
When the stub service exposure is completed, check if(stubService2Flag.get(serviceKey)), then call received(channel, invocation);
There was a problem hiding this comment.
Map<String,Boolean> stubService2Flag is still a variable that needs to be taken care of in case of a memory leak. I think a detailed log giving full information of possible causes would be fine.
There was a problem hiding this comment.
ok, then there is no need to wait(stubServiceToLatch) or delay notification(stubService2Flag). If the stub invoker cannot be obtained, just print the detailed log.
| @@ -702,6 +728,8 @@ public void destroy() { | |||
| } | |||
| } | |||
| referenceClientMap.clear(); | |||
|
|
|||
| stubServiceToLatch.clear(); | |||
There was a problem hiding this comment.
I think we should take destroy of stubServiceToLatch on destroy of stub proxy too.
There might have lots of proxy invokers (reference config) create and destroy during the lifetime of DubboProtocol.
| @@ -253,7 +276,8 @@ private boolean isClientSide(Channel channel) { | |||
| // if it's callback service on client side | |||
| isStubServiceInvoke = Boolean.TRUE.toString().equals(inv.getObjectAttachments().get(STUB_EVENT_KEY)); | |||
| if (isStubServiceInvoke) { | |||
| port = channel.getRemoteAddress().getPort(); | |||
| // The port of the client exposed stub service is 0 | |||
| port = 0; | |||
There was a problem hiding this comment.
One similar PR has been merged into the master branch
| @@ -702,6 +728,8 @@ public void destroy() { | |||
| } | |||
| } | |||
| referenceClientMap.clear(); | |||
|
|
|||
| stubServiceToLatch.clear(); | |||
There was a problem hiding this comment.
I don't think waiting for the stub to be ready is necessary for it will make the whole process more complicated. Just giving a more elegant and clear Exception or result would be enough. What do you think? @BurningCN
|
Let's change this pull request following this patch #9825. Besides that, the only thing that needs to do is to add some extra error information. |
OK |
This reverts commit 8332b99.
This reverts commit d6363e6.
fix apache#9824 (cherry picked from commit 1222676)
When the
consumeropens thestub, the exposure of the stub service (in theStubProxyWrapper#getProxymethod) occurs after the connection between the client and the server is completed, so when theconnectedordisconnectedevent in the client is triggered(DubboProtocol#requestHandler), the stub service cannot be found, so that the related methods of the consumer stub service cannot be called.