Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.0] fix urlInvokerMap concurrent access issue #8701

Merged
merged 15 commits into from
Sep 21, 2021

Conversation

zrlw
Copy link
Contributor

@zrlw zrlw commented Sep 6, 2021

What is the purpose of the change

fix #8675
it might cause chaos that main thread does isAvailable() while main-EventThread does refreshInvoker() if both of them operate urlInvokerMap directly, especially while main-EventThread gets empty url lists to refresh.

github integration test log snippet:

<=== urlInvokerMap will be removed at refreshInvoker() in ServiceDiscoveryRegistryDirectory
[20/09/21 10:06:14:014 UTC] main-EventThread  INFO client.ServiceDiscoveryRegistryDirectory:  [DUBBO] Refreshed invoker size 1, dubbo version: 3.0.3-SNAPSHOT, current host: 172.26.0.3 
...
<=== IllegalStateException will be trigged if urlInvokerMap is empty at checkInvokerAvailable() in ReferenceConfig
[20/09/21 10:06:14:014 UTC] main ERROR deploy.DefaultModuleDeployer:  [DUBBO] Dubbo Module[1.1.1] refer catch error, dubbo version: 3.0.3-SNAPSHOT, current host: 172.26.0.3
java.lang.IllegalStateException: Failed to check the status of the service org.apache.dubbo.samples.async.api.GreetingService. No provider available for the service org.apache.dubbo.samples.async.api.GreetingService from the url dubbo://172.26.0.3/org.apache.dubbo.samples.async.api.GreetingService?application=async-consumer&dubbo=2.0.2&interface=org.apache.dubbo.samples.async.api.GreetingService&metadata-type=remote&methods=greeting,replyGreeting&pid=17&register.ip=172.26.0.3&release=3.0.3-SNAPSHOT&side=consumer&sticky=false&timeout=10000&timestamp=1632132372838 to the consumer 172.26.0.3 use dubbo version 3.0.3-SNAPSHOT
	at org.apache.dubbo.config.ReferenceConfig.checkInvokerAvailable(ReferenceConfig.java:520)

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2021

Codecov Report

Merging #8701 (8bd0479) into 3.0 (d3f7ab4) will increase coverage by 0.05%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##                3.0    #8701      +/-   ##
============================================
+ Coverage     63.77%   63.82%   +0.05%     
+ Complexity      315      314       -1     
============================================
  Files          1169     1169              
  Lines         49216    49222       +6     
  Branches       7361     7359       -2     
============================================
+ Hits          31389    31418      +29     
+ Misses        14406    14381      -25     
- Partials       3421     3423       +2     
Impacted Files Coverage Δ
.../dubbo/registry/integration/RegistryDirectory.java 50.32% <85.71%> (+0.32%) ⬆️
...stry/client/ServiceDiscoveryRegistryDirectory.java 46.25% <87.50%> (-0.18%) ⬇️
...n/java/org/apache/dubbo/rpc/model/ModuleModel.java 83.05% <0.00%> (-9.41%) ⬇️
.../apache/dubbo/remoting/transport/AbstractPeer.java 60.86% <0.00%> (-8.70%) ⬇️
...he/dubbo/remoting/transport/netty/NettyServer.java 70.17% <0.00%> (-3.51%) ⬇️
.../client/metadata/ServiceInstanceMetadataUtils.java 84.90% <0.00%> (-2.84%) ⬇️
.../dubbo/remoting/transport/netty4/NettyChannel.java 64.35% <0.00%> (-1.99%) ⬇️
...a/org/apache/dubbo/monitor/dubbo/DubboMonitor.java 86.66% <0.00%> (-1.91%) ⬇️
.../dubbo/rpc/protocol/dubbo/DecodeableRpcResult.java 51.28% <0.00%> (-1.29%) ⬇️
.../src/main/java/org/apache/dubbo/rpc/RpcStatus.java 70.23% <0.00%> (-1.20%) ⬇️
... and 28 more

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 d3f7ab4...8bd0479. Read the comment docs.

@pinxiong
Copy link
Member

pinxiong commented Sep 7, 2021

Could you provide more detailed info? And why did you use the variable forbidden?

@zrlw
Copy link
Contributor Author

zrlw commented Sep 7, 2021

Could you provide more detailed info? And why did you use the variable forbidden?
it might cause chaos that main thread does isAvailable() while main-EventThread does refreshInvoker() at the same time, especially while main-EventThread gets empty url lists to refresh.
pls see more details in #8675
there are same problem in RegistryDirectory.java. you'd better compare these code with current branch master.

@zrlw zrlw changed the title [3.0] patch service discovery registry directory [3.0] patch urlInvokerMap concurrent access problem Sep 7, 2021
@zrlw zrlw requested a review from AlbumenJ September 11, 2021 15:58
@zrlw zrlw force-pushed the patch-ServiceDiscoveryRegistryDirectory branch from ec2922f to e42d0a1 Compare September 12, 2021 02:15
@zrlw zrlw changed the title [3.0] patch urlInvokerMap concurrent access problem [3.0] fix urlInvokerMap concurrent access issue Sep 16, 2021
@zrlw zrlw requested a review from AlbumenJ September 21, 2021 11:23
@AlbumenJ AlbumenJ merged commit 47e4b81 into apache:3.0 Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.0] ServiceDiscoveryRegistryDirectory的refreshInvoker方法和isAvailable存在并行执行冲突
4 participants