[dubbo 4890]Undeleted invokers(destroyed) In AbstractProxyProtocol may cause memory leak

[finalcola]

What is the purpose of the change

destroyed invokers in AbstractProxyProtocol can't be removed from list.

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 something
            }

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

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]

Codecov Report

Merging #4738 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #4738      +/-   ##
============================================
- Coverage     63.97%   63.94%   -0.03%     
  Complexity      451      451              
============================================
  Files           769      769              
  Lines         33178    33182       +4     
  Branches       5229     5229              
============================================
- Hits          21225    21219       -6     
- Misses         9531     9538       +7     
- Partials       2422     2425       +3

Impacted Files	Coverage Δ	Complexity Δ
...ache/dubbo/rpc/protocol/AbstractProxyProtocol.java	80% <100%> (+1.42%)	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%> (ø)
.../dubbo/remoting/transport/netty4/NettyChannel.java	61.17% <0%> (-4.71%)	0% <0%> (ø)
.../apache/dubbo/remoting/transport/AbstractPeer.java	63.04% <0%> (-4.35%)	0% <0%> (ø)
...he/dubbo/remoting/transport/netty/NettyServer.java	69.64% <0%> (-3.58%)	8% <0%> (-1%)
...c/main/java/org/apache/dubbo/rpc/RpcException.java	80% <0%> (-3.34%)	0% <0%> (ø)
...org/apache/dubbo/rpc/protocol/AbstractInvoker.java	60.65% <0%> (-3.28%)	0% <0%> (ø)
...pache/dubbo/registry/support/AbstractRegistry.java	78.54% <0%> (-3.07%)	0% <0%> (ø)
...exchange/support/header/HeaderExchangeHandler.java	64.75% <0%> (-2.46%)	0% <0%> (ø)
... and 8 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 2c92916...3fcce45. Read the comment docs.

[finalcola]

but Protocol is Singleton, it only remove invoker when shutdown

…

------------------ 原始邮件 ------------------ 发件人: "Taosheng Wei"<notifications@github.com>; 发送时间: 2019年8月21日(星期三) 上午10:22 收件人: "apache/dubbo"<dubbo@noreply.github.com>; 抄送: "final"<845447141@qq.com>; "Author"<author@noreply.github.com>; 主题: Re: [apache/dubbo] [dubbo 4890]Undeleted invokers(destroyed) InAbstractProxyProtocol may cause memory leak (#4738) @tswstarplanet requested changes on this pull request. In dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/protocol/AbstractProxyProtocol.java: @@ -121,6 +121,13 @@ protected Result doInvoke(Invocation invocation) throws Throwable { throw getRpcException(type, url, invocation, e); } } + + @OverRide + public void destroy() { + super.destroy(); + target.destroy(); + invokers.remove(this); + } I think it's unnecessary, because the AbstractProtocol's destroy method has the logic to remove invoker of invokers. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[CLAassistant]

All committers have signed the CLA.