fix #11986 ConcurrentHashMapUtils lock bug #11987

looly · 2023-04-02T13:17:35Z

What is the purpose of the change

fix ConcurrentHashMapUtils lock bug

Brief changelog

Verifying this change

Closed #11986

Checklist

Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
Add some description to dubbo-website project if you are requesting to add a feature.
GitHub Actions works fine on your own branch.
If this contribution is large, please follow the Software Donation Guide.

looly · 2023-04-02T13:19:54Z

issue : #11986

AlbumenJ · 2023-04-03T08:31:44Z

looly · 2023-04-03T12:30:58Z

@AlbumenJ 限定到了jdk8，看行不行。

AlbumenJ · 2023-04-04T09:25:10Z

dubbo-common/src/test/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtilsTest.java

+        // https://github.com/apache/dubbo/issues/11986
+        final ConcurrentHashMap<String,Integer> map=new ConcurrentHashMap<>();
+        // // map.computeIfAbsent("AaAa", key->map.computeIfAbsent("BBBB",key2->42));
+        if (JRE.JAVA_8.isCurrentVersion()) {


这里应该任何版本的 JDK 都应该可以使用吧

并不是。这个bug在JDK8下会造成循环调用，但是jdk9+修复了这个问题，解决方式是有循环调用则抛出异常。这个test在jdk8下通过，jdk9+抛出异常。

那基于 junit5 的注解做两个测试吧
现在 PR的 UT 只跑 JDK17，JDK 8 11 17 的全量测试只有 scheduled 的时候跑

@AlbumenJ 是这个意思么？我拆分成两个test。

@EnabledForJreRange(max = JRE.JAVA_8) @EnabledForJreRange(min = JRE.JAVA_9)

写两个 test，分别通过 EnabledForJreRange 配置运行的 jdk 版本

@AlbumenJ 辛苦看下。

KamToHung

#6032
ExtensionLoader以前也遇到过这种问题，这样修改后代码健壮性挺好的

AlbumenJ · 2023-04-05T06:26:09Z

dubbo-common/src/test/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtilsTest.java

+        // https://github.com/apache/dubbo/issues/11986
+        final ConcurrentHashMap<String,Integer> map=new ConcurrentHashMap<>();
+        // // map.computeIfAbsent("AaAa", key->map.computeIfAbsent("BBBB",key2->42));
+        if (JRE.JAVA_8.isCurrentVersion()) {


那基于 junit5 的注解做两个测试吧
现在 PR的 UT 只跑 JDK17，JDK 8 11 17 的全量测试只有 scheduled 的时候跑

codecov-commenter · 2023-04-05T16:48:16Z

Codecov Report

Merging #11987 (b012096) into 3.2 (8b65828) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                3.2   #11987      +/-   ##
============================================
- Coverage     69.40%   69.35%   -0.06%     
  Complexity        2        2              
============================================
  Files          1605     1605              
  Lines         66165    66171       +6     
  Branches       9698     9701       +3     
============================================
- Hits          45924    45893      -31     
- Misses        15811    15841      +30     
- Partials       4430     4437       +7

see 24 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

AlbumenJ · 2023-04-06T01:51:26Z

dubbo-common/src/test/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtilsTest.java

+        // https://github.com/apache/dubbo/issues/11986
+        final ConcurrentHashMap<String,Integer> map=new ConcurrentHashMap<>();
+        // // map.computeIfAbsent("AaAa", key->map.computeIfAbsent("BBBB",key2->42));
+        if (JRE.JAVA_8.isCurrentVersion()) {


写两个 test，分别通过 EnabledForJreRange 配置运行的 jdk 版本

…o into 3.2-fixConcurrentHashMapUtils

AlbumenJ · 2023-04-13T13:08:18Z

dubbo-common/src/test/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtilsTest.java

+        // JDK8下，由于循环调用bug，会造成执行computeIfAbsent死循环问题
+        // ConcurrentHashMapUtils.computeIfAbsent用于解决此问题，保证正常执行


Please comment in English

@AlbumenJ check it, please.

…o into 3.2-fixConcurrentHashMapUtils

AlbumenJ · 2023-04-20T07:56:16Z

dubbo-common/src/main/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtils.java

+                // v = map.computeIfAbsent(key, func);
+
+                // this bug fix methods maybe cause `func.apply` multiple calls.
+                v = func.apply(key);


Will this allocate more memory?

AlbumenJ

LGTM.
@chickenlj @CrazyHZM PTAL

dubbo-common/src/main/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtils.java

…o into 3.2-fixConcurrentHashMapUtils

sonarcloud · 2023-05-06T18:24:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

9.1% Coverage
0.0% Duplication

looly added 3 commits April 2, 2023 21:02

fix apache#11986

725fe23

fix apache#11986

f6ad595

fix apache#11986

85e23cb

looly changed the title ~~fix ConcurrentHashMapUtils lock bug~~ fix #11986 ConcurrentHashMapUtils lock bug Apr 2, 2023

looly added 2 commits April 3, 2023 20:03

change test at jdk8

ce40cb8

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

0f079b7

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

585d2df

AlbumenJ reviewed Apr 4, 2023

View reviewed changes

KamToHung reviewed Apr 4, 2023

View reviewed changes

AlbumenJ requested changes Apr 5, 2023

View reviewed changes

looly and others added 2 commits April 6, 2023 00:01

seprate test for jdk8 and jdk17

ac575f3

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

f93fa99

AlbumenJ requested changes Apr 6, 2023

View reviewed changes

looly and others added 3 commits April 12, 2023 21:04

seprate jre version test

71e6f31

Merge branch '3.2-fixConcurrentHashMapUtils' of github.com:looly/dubb…

4fcc594

…o into 3.2-fixConcurrentHashMapUtils

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

6a87260

looly requested review from AlbumenJ and KamToHung April 12, 2023 13:06

AlbumenJ requested changes Apr 13, 2023

View reviewed changes

looly and others added 3 commits April 13, 2023 22:24

change comment to english

b6401b2

Merge branch '3.2-fixConcurrentHashMapUtils' of github.com:looly/dubb…

1c968bd

…o into 3.2-fixConcurrentHashMapUtils

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

3405965

looly requested a review from AlbumenJ April 13, 2023 14:28

looly and others added 3 commits April 14, 2023 22:32

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

18fef5d

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

968dcb2

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

fcb61b8

AlbumenJ reviewed Apr 20, 2023

View reviewed changes

AlbumenJ approved these changes Apr 20, 2023

View reviewed changes

CrazyHZM requested changes May 6, 2023

View reviewed changes

dubbo-common/src/main/java/org/apache/dubbo/common/utils/ConcurrentHashMapUtils.java Show resolved Hide resolved

looly and others added 3 commits May 7, 2023 01:21

add null check for v

e2fa282

Merge branch '3.2-fixConcurrentHashMapUtils' of github.com:looly/dubb…

6be0560

…o into 3.2-fixConcurrentHashMapUtils

Merge branch '3.2' into 3.2-fixConcurrentHashMapUtils

b012096

AlbumenJ requested a review from CrazyHZM May 7, 2023 12:47

CrazyHZM approved these changes May 8, 2023

View reviewed changes

AlbumenJ merged commit 0c11a0d into apache:3.2 May 10, 2023
14 checks passed

This was referenced Jun 9, 2023

[ISSUE #6879] ConcurrentHashMapUtils fails to solve the loop bug in JDK8 apache/rocketmq#6883

Merged

fix: CollectionUtils#computeIf lock bug baomidou/mybatis-plus#5399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #11986 ConcurrentHashMapUtils lock bug #11987

fix #11986 ConcurrentHashMapUtils lock bug #11987

looly commented Apr 2, 2023 •

edited

looly commented Apr 2, 2023

AlbumenJ commented Apr 3, 2023

looly commented Apr 3, 2023

AlbumenJ Apr 4, 2023

looly Apr 4, 2023

AlbumenJ Apr 5, 2023

looly Apr 5, 2023

AlbumenJ Apr 6, 2023

AlbumenJ Apr 6, 2023

looly Apr 13, 2023

KamToHung left a comment

AlbumenJ Apr 5, 2023

codecov-commenter commented Apr 5, 2023 •

edited

AlbumenJ Apr 6, 2023

AlbumenJ Apr 13, 2023

looly Apr 13, 2023

AlbumenJ Apr 20, 2023

looly Apr 20, 2023

AlbumenJ left a comment

sonarcloud bot commented May 6, 2023

		// JDK8下，由于循环调用bug，会造成执行computeIfAbsent死循环问题
		// ConcurrentHashMapUtils.computeIfAbsent用于解决此问题，保证正常执行

fix #11986 ConcurrentHashMapUtils lock bug #11987

fix #11986 ConcurrentHashMapUtils lock bug #11987

Conversation

looly commented Apr 2, 2023 • edited

What is the purpose of the change

Brief changelog

Verifying this change

Checklist

looly commented Apr 2, 2023

AlbumenJ commented Apr 3, 2023

looly commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KamToHung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbumenJ left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 6, 2023

looly commented Apr 2, 2023 •

edited

codecov-commenter commented Apr 5, 2023 •

edited