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

Fix the issue that the ReferenceConfigCache#destroy method does not call proxy.$destroy() #8065

Merged

Conversation

@BurningCN
Copy link
Contributor

@BurningCN BurningCN commented Jun 16, 2021

What is the purpose of the change

The destroy method should trigger the call of proxy.$destroy() like the destroyAll method
Uploading image.png…

@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Jun 16, 2021

Codecov Report

Merging #8065 (83e882b) into master (9f6b536) will increase coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8065      +/-   ##
============================================
+ Coverage     60.85%   61.10%   +0.25%     
+ Complexity      491      490       -1     
============================================
  Files          1089     1091       +2     
  Lines         43876    43954      +78     
  Branches       6413     6417       +4     
============================================
+ Hits          26699    26860     +161     
+ Misses        14208    14109      -99     
- Partials       2969     2985      +16     
Impacted Files Coverage Δ
...pache/dubbo/config/utils/ReferenceConfigCache.java 61.17% <100.00%> (+0.46%) ⬆️
...in/java/org/apache/dubbo/common/utils/JVMUtil.java 81.13% <0.00%> (-11.33%) ⬇️
...che/dubbo/remoting/transport/mina/MinaChannel.java 35.52% <0.00%> (-10.53%) ⬇️
...ting/exchange/support/header/HeartbeatHandler.java 83.72% <0.00%> (-6.98%) ⬇️
...n/java/org/apache/dubbo/common/utils/LFUCache.java 80.86% <0.00%> (-5.42%) ⬇️
...port/identifier/BaseServiceMetadataIdentifier.java 53.57% <0.00%> (-3.58%) ⬇️
...etadata/report/support/AbstractMetadataReport.java 57.28% <0.00%> (-3.40%) ⬇️
.../remoting/transport/netty4/NettyClientHandler.java 83.05% <0.00%> (-3.39%) ⬇️
...e/dubbo/remoting/exchange/codec/ExchangeCodec.java 77.73% <0.00%> (-2.43%) ⬇️
...pache/dubbo/registry/support/AbstractRegistry.java 77.40% <0.00%> (-1.49%) ⬇️
... and 40 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 9f6b536...83e882b. Read the comment docs.

@@ -175,7 +175,8 @@ public void destroy(String key, Class<?> type) {

Map<String, Object> proxiesOftype = proxies.get(type);
if (CollectionUtils.isNotEmptyMap(proxiesOftype)) {
proxiesOftype.remove(key);
Destroyable proxy = (Destroyable) proxiesOftype.remove(key);
Copy link
Member

@AlbumenJ AlbumenJ Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be better to check if the object is an instance of Destroyable first

Copy link
Contributor Author

@BurningCN BurningCN Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated proxy instances all automatically implement the Destroyable interface, right?

image
image

@AlbumenJ AlbumenJ merged commit eb9917e into apache:master Jun 24, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants