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

Resolving the Thread - Safety Problem in registerReferenceKeyAndBeanName with Overloaded computeIfAbsent #15219

Open
wants to merge 10 commits into
base: 3.3
Choose a base branch
from

Conversation

juzzia
Copy link

@juzzia juzzia commented Mar 8, 2025

org.apache.dubbo.config.spring.reference.ReferenceBeanManager.java

    public void registerReferenceKeyAndBeanName(String referenceKey, String referenceBeanNameOrAlias) {
        List<String> list =
                ConcurrentHashMapUtils.computeIfAbsent(referenceKeyMap, referenceKey, (key) -> new ArrayList<>());
        if (!list.contains(referenceBeanNameOrAlias)) {
            list.add(referenceBeanNameOrAlias);
            // register bean name as alias
            referenceAliasMap.put(referenceBeanNameOrAlias, list.get(0));
        }
    }

The ConcurrentHashMapUtils.computeIfAbsent method ensures the creation of a new list in a thread-safe manner when no list exists for the given referenceKey. However, the subsequent operations if (!list.contains(referenceBeanNameOrAlias)) and list.add(referenceBeanNameOrAlias) are not atomic. This situation can lead to thread-safety issues where multiple threads might simultaneously pass the contains check and then all perform the add operation, resulting in duplicate referenceBeanNameOrAlias entries within the list.

To resolve this issue, I added an overloaded version of the computeIfAbsent method that allows for thread-safe operations on the value through the use of a Consumer functional interface. Additionally, I provided concurrent unit tests:

  • ConcurrentHashMapUtilsTest#threadSafetyOperatorForJava8Test
  • ConcurrentHashMapUtilsTest#threadSafetyOperatorForJava17Test
    These tests confirm the correctness of the implementation.

The profile defined here cannot run the spotless plugin in JDK 8 because the version of the Spotless plugin is too high. Therefore, I've overridden the version of the Spotless plugin in the jdk8 - jdk11 profile to make it compatible with the lower - version environment. spotless plugin version 2.22.0 can work properly in JDK versions ranging from 8 to 11.

    <profile>
      <id>jdk9-jdk11-spotless</id>
      <activation>
        <jdk>[1.8, 11)</jdk>
      </activation>
      <properties>
        <palantirJavaFormat.version>1.1.0</palantirJavaFormat.version>
      </properties>
    </profile>
    <properties>
      <spotless-maven-plugin.lower.version>2.22.0</spotless-maven-plugin.lower.version>
    </properties>
    <profile>
      <id>jdk8-jdk11-spotless</id>
      <activation>
        <jdk>[1.8, 11)</jdk>
      </activation>
      <properties>
        <palantirJavaFormat.version>1.1.0</palantirJavaFormat.version>
        <spotless-maven-plugin.version>${spotless-maven-plugin.lower.version}</spotless-maven-plugin.version>
      </properties>
    </profile>

Use a lower version of the spotless-maven-plugin dependency to override the version defined by spotless-maven-plugin.version so as to support JDK 1.8.

juzzia added 7 commits March 8, 2025 12:12
    public void registerReferenceKeyAndBeanName(String referenceKey, String referenceBeanNameOrAlias) {
        List<String> list =
                ConcurrentHashMapUtils.computeIfAbsent(referenceKeyMap, referenceKey, (key) -> new ArrayList<>());
        if (!list.contains(referenceBeanNameOrAlias)) {
            list.add(referenceBeanNameOrAlias);
            // register bean name as alias
            referenceAliasMap.put(referenceBeanNameOrAlias, list.get(0));
        }
    }
```
The ConcurrentHashMapUtils.computeIfAbsent method ensures the creation of a new list in a thread-safe manner when no list exists for the given referenceKey. However, the subsequent operations if (!list.contains(referenceBeanNameOrAlias)) and list.add(referenceBeanNameOrAlias) are not atomic. This situation can lead to thread-safety issues where multiple threads might simultaneously pass the contains check and then all perform the add operation, resulting in duplicate referenceBeanNameOrAlias entries within the list.

To resolve this issue, I added an overloaded version of the computeIfAbsent method that allows for thread-safe operations on the value through the use of a Consumer<T> functional interface. Additionally, I provided concurrent unit tests:

ConcurrentHashMapUtilsTest#threadSafetyOperatorForJava8Test
ConcurrentHashMapUtilsTest#threadSafetyOperatorForJava17Test
These tests confirm the correctness of the implementation.
@AlbumenJ
Copy link
Member

image

@juzzia
Copy link
Author

juzzia commented Mar 11, 2025

image

Thank you very much for the screenshot 👍. Due to continuous errors when downloading local Maven dependencies, the project still cannot run mvn spotless:apply. Therefore, I have been trying to push my code to check if my code formatting passes the checks 😭. I will use your screenshot as a reference and try to reformat my code again.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 35.60%. Comparing base (d3b1e1f) to head (9eb5a78).

Files with missing lines Patch % Lines
...che/dubbo/common/utils/ConcurrentHashMapUtils.java 90.90% 0 Missing and 1 partial ⚠️
.../config/spring/reference/ReferenceBeanManager.java 90.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (d3b1e1f) and HEAD (9eb5a78). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (d3b1e1f) HEAD (9eb5a78)
unit-tests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##                3.3   #15219       +/-   ##
=============================================
- Coverage     60.79%   35.60%   -25.19%     
+ Complexity    10892    10881       -11     
=============================================
  Files          1885     1885               
  Lines         86082    86091        +9     
  Branches      12895    12895               
=============================================
- Hits          52331    30657    -21674     
- Misses        28303    50989    +22686     
+ Partials       5448     4445     -1003     
Flag Coverage Δ
integration-tests 33.07% <85.71%> (-0.08%) ⬇️
samples-tests 29.21% <90.47%> (+0.02%) ⬆️
unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

juzzia and others added 2 commits March 13, 2025 01:53
 Solve the issue where the commands mvn spotless:apply and mvn spotless:check cannot be executed in a JDK 8 environment.

Use a lower version of the spotless-maven-plugin dependency to override the version defined by spotless-maven-plugin.version so as to support JDK 1.8.
@juzzia
Copy link
Author

juzzia commented Mar 13, 2025

@AlbumenJ I'm not sure why the workflow execution timed out since my code changes should not affect the test runs of the AbstractDynamicConfigurationTest class. I suspect it might be due to dependent services. Could you please rerun the PR action?

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 participants