Motivation:

Can completely solve the OOM problem caused by {@link java.util.concurrent.LinkedBlockingQueue}

Modification and Result:

CN

参考dubbo的思想(dubbo-pr)，引入MemorySafeLinkedBlockingQueue，解决无界队列可能会导致OOM的问题

EN

Referring to the idea of dubbo（dubbo-pr）, introduce MemorySafeLinkedBlockingQueue to solve the problem that unbounded queues may cause OOM

Jun 10 '22 02:06 Codeprh

Hi @Codeprh, welcome to SOFAStack community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

Jun 10 '22 02:06 sofastack-bot[bot]

Hi @Codeprh, thank for your PR. Please run mvn clean compile -DskipTests before you commit to format the code.

Jun 24 '22 03:06 EvenLjj

mvn clean compile -DskipTests

I'm done, I think there is no problem with my code, please help me, thank you~

Jul 05 '22 14:07 Codeprh

Hi @Codeprh, thank for your PR. Please run mvn clean compile -DskipTests before you commit to format the code.

done,3q

Jul 06 '22 06:07 Codeprh

Codecov Report

Merging #1213 (ae2e21f) into master (bb7c51b) will increase coverage by 0.52%. The diff coverage is 56.25%.

@@             Coverage Diff              @@
##             master    #1213      +/-   ##
============================================
+ Coverage     71.56%   72.09%   +0.52%     
+ Complexity      832      780      -52     
============================================
  Files           408      412       +4     
  Lines         17224    17403     +179     
  Branches       2684     2702      +18     
============================================
+ Hits          12327    12546     +219     
+ Misses         3533     3475      -58     
- Partials       1364     1382      +18

Impacted Files	Coverage Δ
...mmon/threadpool/MemorySafeLinkedBlockingQueue.java	`50.00% <50.00%> (ø)`
...a/rpc/common/threadpool/MemoryLimitCalculator.java	`61.53% <61.53%> (ø)`
.../alipay/sofa/rpc/common/utils/ThreadPoolUtils.java	`95.00% <100.00%> (ø)`
...ofa/rpc/registry/zk/ZookeeperProviderObserver.java	`72.50% <0.00%> (-2.50%)`	:arrow_down:
...a/com/alipay/sofa/rpc/event/LookoutSubscriber.java	`88.63% <0.00%> (-2.28%)`	:arrow_down:
.../alipay/sofa/rpc/metrics/lookout/RpcLookoutId.java	`87.30% <0.00%> (-1.59%)`	:arrow_down:
...lipay/sofa/rpc/message/AbstractResponseFuture.java	`56.14% <0.00%> (-0.88%)`	:arrow_down:
.../java/com/alipay/sofa/rpc/common/RpcConstants.java	`100.00% <0.00%> (ø)`
...alipay/sofa/rpc/registry/local/DomainRegistry.java	`94.11% <0.00%> (ø)`
.../sofa/rpc/registry/local/DomainRegistryHelper.java	`62.50% <0.00%> (ø)`
... and 15 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 bb7c51b...ae2e21f. Read the comment docs.

Jul 09 '22 05:07 codecov[bot]

also see : https://github.com/apache/dubbo/pull/9789

Jul 15 '22 09:07 OrezzerO

@OrezzerO @Codeprh How about this PR: https://github.com/apache/shenyu/pull/3764 ?

Jul 27 '22 10:07 loongs-zhang

@OrezzerO @Codeprh How about this PR: apache/shenyu#3764 ?

This PR has better handling of exceptions. 👍

Aug 09 '22 09:08 JervyShi

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Nov 12 '22 11:11 stale[bot]

review again

May 05 '23 00:05 nobodyiam

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

Overall summary:

This pull request introduces the "feat: add MemorySafeLinkedBlockingQueue" feature, which involves the creation of the MemoryLimitCalculator class and modifications to the MemorySafeLinkedBlockingQueue, MemoryLimiter, ThreadPoolUtils, and related test files. The primary goal of these changes is to address the OutOfMemory (OOM) problem caused by java.util.concurrent.LinkedBlockingQueue.

Potential issues:

NullPointerException might be thrown during object acquisition in the MemoryLimiter class. Proper input validation should be performed.
Ensure that the MemoryLimitCalculator's scheduled refresh of maximum available memory does not cause performance issues.
Verify concurrency, performance, and accuracy of computation of memory usage in MemoryLimiter.
Check the test cases included in MemorySafeLinkedBlockingQueueTest to confirm adequate testing coverage and reliability.
Ensure the proper removal of the MemoryLimiter class and verify if there are any remaining dependencies on this class.
Ensure that the OOM problem is completely solved by the changes made to the MemoryLimitCalculator class.

Key findings:

The MemoryLimitCalculator class has been created for calculating and periodically refreshing maximum available memory.
The MemoryLimiter class is no longer needed and has been removed, but its removal's impact on the codebase should be double-checked.
The MemorySafeLinkedBlockingQueue class has been created as an implementation of the queue using the MemoryLimitCalculator.
Test cases have been added and modified, but it is important to ensure that adequate testing coverage and reliability is maintained for the new changes.

Recommendations:

Check for any remaining dependencies on the MemoryLimiter class and ensure that its removal does not cause any integration issues.
Thoroughly test the changes made to the MemoryLimitCalculator class to confirm that the OOM problem is indeed solved under different scenarios.
Make sure that there is proper locking and unlocking for acquiring or releasing objects in memory, and no thread safety issues are present.
Verify that the testing coverage is adequate, especially for the new changes in the MemorySafeLinkedBlockingQueue file.

Details

Commit b5fd13d458ba25ab08579a5b5149c0a1d80ea83f

Summary of key changes:

Creation of MemoryLimitCalculator class to calculate and periodically refresh the maximum available memory.
Creation of MemoryLimiter class to handle acquiring and releasing objects in memory.
Creation of MemorySafeLinkedBlockingQueue class, which is a queue implementation that uses the MemoryLimiter.
Added a new test for MemorySafeLinkedBlockingQueue (MemorySafeLinkedBlockingQueueTest).
Minor update in ThreadPoolUtils class.

Key findings and potential problems:

NullPointerException might be thrown during object acquisition in the MemoryLimiter class. Ensure that proper input validation is performed.
Ensure that the MemoryLimitCalculator's scheduled refresh of maximum available memory does not cause performance issues.
While acquiring or releasing objects in memory, ensure that proper locking and unlocking are performed, and no thread safety issues are present.
Verify concurrency, performance, and accuracy of computation of memory usage in MemoryLimiter.
Check the test cases included in MemorySafeLinkedBlockingQueueTest to confirm adequate testing coverage and reliability.

Commit 94e41ca93544c57a864a64a76277efb7914e595e

This pull request makes changes to 4 files related to the MemorySafeLinkedBlockingQueue implementation:

Key changes:

The MemoryLimitCalculator.java file has only a line removed which doesn't seem to impact any functionality.
The MemoryLimiter.java file had some formatting changes (e.g., adjustments to field alignment), but no changes to the logic.
The key changes are in the implementation of the MemorySafeLinkedBlockingQueue.java file, where the static variable THE_256_MB is set to 256MB, and the constructor with no arguments initializes the instance variable maxFreeMemory with the value of THE_256_MB.
The MemorySafeLinkedBlockingQueueTest.java file has new license comment block and additional lines of whitespace but no changes in the unit tests.

Potential problems:

No significant updates to the testing coverage to account for new changes in the MemorySafeLinkedBlockingQueue.java file.

Commit 622ba0a251513b3d54a25ce31135fab09c21fc08

This patch is for the file ThreadPoolUtilsTest.java and modifies the test cases for the buildQueue and buildQueue1 methods. The key changes are:

Import statement for MemorySafeLinkedBlockingQueue has been added.
Two assertions are updated to expect the MemorySafeLinkedBlockingQueue class instead of the LinkedBlockingQueue class for the cases with a capacity of -1 in both buildQueue and buildQueue1 test methods.

Potential problems:

The actual implementation of MemorySafeLinkedBlockingQueue or its integration with ThreadPoolUtils is not present in this patch. Ensure that these changes are present in the complete Pull Request or other related patches.
The test suite may lack sufficient testing or edge cases for the MemorySafeLinkedBlockingQueue implementation, depending on its complexity and possible side effects on the ThreadPoolUtils.

Commit ae2e21f554e0f7317c7c73eedfd245be769403b1

Summary of key changes in the patch:

Removal of MemoryLimiter class (core/common/src/main/java/com/alipay/sofa/rpc/common/threadpool/MemoryLimiter.java).
Modification of the MemoryLimitCalculator class
- Removal of its dependency on MemoryLimiter.
- Update of JavaDoc to mention it can completely solve the OOM problem caused by java.util.concurrent.LinkedBlockingQueue and does not depend on java.lang.instrument.Instrumentation.

Potential problems:

The removal of the MemoryLimiter class can have a substantial impact on the codebase, and it's essential to ensure that there are no dependencies on this class. As it's not clear whether or not other parts of the code still depend on MemoryLimiter, it's essential to check for any possible integration issues.
While the updated JavaDoc for MemoryLimitCalculator mentions that it can completely solve the OOM problem, it's critical to test the code under different scenarios where an OOM problem could occur, ensuring its correctness and effectiveness. This is important because solving the OOM problem is one of the main goals of this pull request.

Based on this patch, I recommend checking for any dependencies on the MemoryLimiter class and making sure that the supposed OOM problem is indeed solved by the changes made to the MemoryLimitCalculator class.

May 05 '23 00:05 nobodyiam