[dubbo 4458] doRefer in RestProtocol may cause OOM #4629

finalcola · 2019-07-22T10:07:39Z

What is the purpose of the change

clear resouces when invoker(AbstructProxyInvoker) is not in use:

protected <T> Invoker<T> protocolBindingRefer(final Class<T> type, final URL url) throws RpcException {
        final Invoker<T> target = proxyFactory.getInvoker(doRefer(type, url), type, url);
        Invoker<T> invoker = new AbstractInvoker<T>(type, url) {
            @Override
            protected Result doInvoke(Invocation invocation) throws Throwable {
                // do invoke...
            }

            // invokers need clear unused invoker
            @Override
            public void destroy() {
                super.destroy();
                target.destroy();
                invokers.remove(this);
            }
        };
        invokers.add(invoker);
        return invoker;
    }

clear RestProtocol.cliens by using map(key is url, value is client)

Brief changelog

XXXXX

Verifying this change

XXXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

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.
Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. 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.
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.
Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
If this contribution is large, please follow the Software Donation Guide.

codecov-io · 2019-07-23T11:11:02Z

Codecov Report

Merging #4629 into master will increase coverage by 0.5%.
The diff coverage is 80.95%.

@@             Coverage Diff             @@
##             master    #4629     +/-   ##
===========================================
+ Coverage     63.48%   63.99%   +0.5%     
- Complexity      451      452      +1     
===========================================
  Files           789      769     -20     
  Lines         34721    33186   -1535     
  Branches       5422     5231    -191     
===========================================
- Hits          22044    21238    -806     
+ Misses        10151     9525    -626     
+ Partials       2526     2423    -103

Impacted Files	Coverage Δ	Complexity Δ
...g/apache/dubbo/rpc/protocol/rest/RestProtocol.java	`70.07% <80.95%> (+1.85%)`	`0 <0> (ø)`	⬇️
.../remoting/transport/netty4/NettyServerHandler.java	`61.9% <0%> (-9.53%)`	`0% <0%> (ø)`
...beans/factory/annotation/ReferenceBeanBuilder.java	`68% <0%> (-7%)`	`0% <0%> (ø)`
...ng/transport/dispatcher/all/AllChannelHandler.java	`51.42% <0%> (-5.72%)`	`0% <0%> (ø)`
.../org/apache/dubbo/remoting/ExecutionException.java	`15.78% <0%> (-5.27%)`	`0% <0%> (ø)`
.../apache/dubbo/remoting/transport/AbstractPeer.java	`58.69% <0%> (-4.35%)`	`0% <0%> (ø)`
...g/p2p/exchange/support/MulticastExchangeGroup.java	`72.34% <0%> (-4.26%)`	`0% <0%> (ø)`
...mmon/serialize/kryo/utils/AbstractKryoFactory.java	`75% <0%> (-3.58%)`	`0% <0%> (ø)`
...ava/org/apache/dubbo/config/ApplicationConfig.java	`88.33% <0%> (-2.9%)`	`0% <0%> (ø)`
...exchange/support/header/HeaderExchangeHandler.java	`64.75% <0%> (-2.46%)`	`0% <0%> (ø)`
... and 52 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 443b6ff...821a9d5. Read the comment docs.

beiwei30 · 2019-07-25T03:37:49Z

@finalcola I am not persuaded that the current solution is better than the original. The reason is, I believe 'connection manager' plus 'connection monitor' will reuse and recycle the client. The potential OOM issue may caused by 'connection manager' is not used when the client is created, which has in fact been fixed by you with another pull request: #4614

beiwei30

not sure this pull request is necessary.

finalcola · 2019-07-25T04:20:17Z

#4614 will clear HTTPConnection, but RestProtocol will keep too much client(ResteasyClient) reference which makes these ResteasyClient instance can't be GC.

finalcola · 2019-07-25T04:20:40Z

@beiwei30

beiwei30 · 2019-07-25T05:18:45Z

I see. In your solution, when will ResteasyClient be recycled?

beiwei30 · 2019-07-25T05:59:55Z

#4614 will clear HTTPConnection, but RestProtocol will keep too much client(ResteasyClient) reference which makes these ResteasyClient instance can't be GC.

a second thought: If I don't misunderstand, there's no way to recycle the clients, even if they are now saved in a map. This may lead to a potential problem, which is, the underlaying connection can be closed by connection manager (see connection monitor). If this happens, this will lead to invocation failure since the resteasy client maintained in the map doesn't have lifcecycle management.

Should we simply create Resteasy client every time and then drop it? I think it is not expensive since the connection will be reused, what do you think?

Or should we maintain the client in weak hash map instead?

finalcola · 2019-07-26T03:58:09Z

The previous implementation was to use a List to collect clients, but add client only, never remove. Now clients are saved in map which means they are in use, and old client will be remove and close. When doRefer happens, will create a new client for this url. @beiwei30

beiwei30 · 2019-07-29T06:22:27Z

The previous implementation was to use a List to collect clients, but add client only, never remove. Now clients are saved in map which means they are in use, and old client will be remove and close. When doRefer happens, will create a new client for this url. @beiwei30

could you pls. point out when the client will be removed from the map? @finalcola

finalcola · 2019-07-29T06:40:18Z

it will not remove. My original idea was that each existing url would generate only one client（previous implementation using Sync List will add client everytime）, because It is not clear whether the client corresponding to the url is still in use.

maybe sync WeakHashMap can solve it, but it dose not pass the travis-ci in JDK 11.
https://travis-ci.org/apache/dubbo/builds/563891937

@beiwei30

finalcola · 2019-07-30T03:32:22Z

now, i use WeakRefence to hold client

finalcola · 2019-08-02T07:36:04Z

@beiwei30 could you please have a look, if there are any problems

CLAassistant · 2019-09-06T10:25:08Z

All committers have signed the CLA.

xiantang · 2020-03-13T15:55:02Z

dubbo-rpc/dubbo-rpc-rest/src/main/java/org/apache/dubbo/rpc/protocol/rest/RestProtocol.java

+
+        public void addConnectionManager(String key, PoolingHttpClientConnectionManager connectionManager) {
+            connectionManagers.put(key, connectionManager);
+//            connectionManagers.add(connectionManager);


remove unuse comment

AlbumenJ · 2021-04-11T09:36:52Z

@finalcola Hi, thanks for your contribution.

Please merge the latest master branch to resolve confilcts.

AlbumenJ · 2021-05-22T07:39:04Z

Close for long time no response.

Please feel free to reopen if you have any question.

If you think these changes still useful in the latest master branch, please submit a new pull request.

codecov-commenter · 2024-08-28T02:49:37Z

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.99%. Comparing base (443b6ff) to head (821a9d5).

Files	Patch %	Lines
...g/apache/dubbo/rpc/protocol/rest/RestProtocol.java	80.95%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4629      +/-   ##
============================================
+ Coverage     63.48%   63.99%   +0.50%     
- Complexity      451      452       +1     
============================================
  Files           789      769      -20     
  Lines         34721    33186    -1535     
  Branches       5422     5231     -191     
============================================
- Hits          22044    21238     -806     
+ Misses        10151     9525     -626     
+ Partials       2526     2423     -103

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

finalcola added 3 commits July 22, 2019 17:44

fix RestProtocol refer bug

9e03149

call targetInvoker.destroy()

07f6b65

del unused import

88e2696

finalcola closed this Jul 23, 2019

finalcola reopened this Jul 23, 2019

beiwei30 requested changes Jul 25, 2019

View reviewed changes

finalcola added 4 commits July 25, 2019 16:14

create ResteasyClient everytime doRefer

452d571

update

97849ee

delete unused imports

3a31841

clear invoker list

8706eef

finalcola added 2 commits July 26, 2019 15:21

use sync WeakHashMap to hold client

9fc2e27

create new client everytime

a120be7

use sync weakHashMap

46dfb27

finalcola closed this Jul 30, 2019

finalcola reopened this Jul 30, 2019

将AbstractProxyProtocol的修改放入其他分支

821a9d5

finalcola closed this Aug 4, 2019

finalcola reopened this Aug 4, 2019

xiantang reviewed Mar 13, 2020

View reviewed changes

AlbumenJ added the status/waiting-for-feedback Need reporters to triage label Apr 11, 2021

AlbumenJ closed this May 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dubbo 4458] doRefer in RestProtocol may cause OOM #4629

[dubbo 4458] doRefer in RestProtocol may cause OOM #4629

finalcola commented Jul 22, 2019 •

edited by beiwei30

Loading

codecov-io commented Jul 23, 2019 •

edited

Loading

beiwei30 commented Jul 25, 2019

beiwei30 left a comment

finalcola commented Jul 25, 2019

finalcola commented Jul 25, 2019

beiwei30 commented Jul 25, 2019

beiwei30 commented Jul 25, 2019

finalcola commented Jul 26, 2019

beiwei30 commented Jul 29, 2019 •

edited

Loading

finalcola commented Jul 29, 2019

finalcola commented Jul 30, 2019

finalcola commented Aug 2, 2019

CLAassistant commented Sep 6, 2019 •

edited

Loading

xiantang Mar 13, 2020

AlbumenJ commented Apr 11, 2021

AlbumenJ commented May 22, 2021

codecov-commenter commented Aug 28, 2024

[dubbo 4458] doRefer in RestProtocol may cause OOM #4629

[dubbo 4458] doRefer in RestProtocol may cause OOM #4629

Conversation

finalcola commented Jul 22, 2019 • edited by beiwei30 Loading

What is the purpose of the change

Brief changelog

Verifying this change

codecov-io commented Jul 23, 2019 • edited Loading

Codecov Report

beiwei30 commented Jul 25, 2019

beiwei30 left a comment

Choose a reason for hiding this comment

finalcola commented Jul 25, 2019

finalcola commented Jul 25, 2019

beiwei30 commented Jul 25, 2019

beiwei30 commented Jul 25, 2019

finalcola commented Jul 26, 2019

beiwei30 commented Jul 29, 2019 • edited Loading

finalcola commented Jul 29, 2019

finalcola commented Jul 30, 2019

finalcola commented Aug 2, 2019

CLAassistant commented Sep 6, 2019 • edited Loading

xiantang Mar 13, 2020

Choose a reason for hiding this comment

AlbumenJ commented Apr 11, 2021

AlbumenJ commented May 22, 2021

codecov-commenter commented Aug 28, 2024

Codecov Report

finalcola commented Jul 22, 2019 •

edited by beiwei30

Loading

codecov-io commented Jul 23, 2019 •

edited

Loading

beiwei30 commented Jul 29, 2019 •

edited

Loading

CLAassistant commented Sep 6, 2019 •

edited

Loading