[Issue 10010][Client] fixed memory leak #10028

abhilashmandaliya · 2021-03-24T04:27:38Z

trying to close all the resources even if previous one fails during close/shutdown
code clean up as per the idea suggestions

Fixes #10010

Motivation

Fixed memory leak caused by reception thrown from the PulsarClientImpl constructor in some cases. Resources were not getting closed which used to end up throwing OOM if user code tries to create a client unless succeeded.

Modifications

Handled exception in the constructor with try-catch and updated shutdown method to close all resources correctly. Previously, it was not attempting to close further resource if the previous one failed. Now it will try to close all of them individully.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no): no
The public API: (yes / no): no
The schema: (yes / no / don't know): no
The default values of configurations: (yes / no): no
The wire protocol: (yes / no): no
The rest endpoints: (yes / no): no
The admin cli options: (yes / no): no
Anything that affects deployment: (yes / no / don't know): no

Documentation

Does this pull request introduce a new feature? (yes / no): no
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented): not applicable
If a feature is not applicable for documentation, explain why?: nothing to concerned with the end-user
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation: not required

MarvinCai

Thanks for working on this, can you add some unit test around it? Like test throwing exception while creating pulsarclient and verify resources are properly released.

MarvinCai · 2021-03-24T05:13:03Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

+            state.set(State.Open);
+        } catch (Throwable t) {
+            shutdown();
+            shutdownEventLoopGroup(eventLoopGroup);


Is this redundant? We already call shutdownEventLoopGroup(eventLoopGroup) in shutDown()

this call is for the eventLoopGroup passed as a constructor argument. if an exception occurs before we assign it to our class member, we will need to close the parameter. once assigned, they will be the same and hence I have added a check whether it's already shutdown or not

MarvinCai · 2021-03-24T05:14:29Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

+            try {
+                // Shutting down eventLoopGroup separately because in some cases, cnxPool might be using different
+                // eventLoopGroup.
+                shutdownEventLoopGroup(eventLoopGroup);


If connectionPool and PulsarClient is using the same eventLoopGroup, this might cause call shutDown on it multiple times, not sure what will happen in this situation? Probably move it to after closing cnxPool?

shouldn't cause any issue as I am checking for already shutdown case. If so, I will not attempt to shut it down. Though I can move it below if you suggest.

codelipenghui

Could you please add some tests for the new change to avoid the regression?

abhilashmandaliya · 2021-03-25T12:04:54Z

Could you please add some tests for the new change to avoid the regression?

@codelipenghui @MarvinCai I have added some tests.

I tried to run some of the failed tests from the previous build but they passed on my local system

codelipenghui · 2021-03-26T01:45:47Z

/pulsarbot run-failure-checks

MarvinCai · 2021-03-26T06:39:38Z

/pulsarbot run-failure-checks

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

315157973

Sorry, the shutdown of a Producer will cause other Consumers and Producers to also shut down. This will be incompatible with the current behavior. @codelipenghui

abhilashmandaliya · 2021-03-26T12:45:30Z

Sorry, the shutdown of a Producer will cause other Consumers and Producers to also shut down. This will be incompatible with the current behavior. @codelipenghui

@315157973 this is the elaborated version of your previous comment right? Now, we are not calling the close method so producers and consumers won't be shut down.

tests are still failing :(

315157973 · 2021-03-26T13:00:36Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

-            lookup.close();
-            cnxPool.close();
-            timer.stop();
-            externalExecutorProvider.shutdownNow();
-            internalExecutorService.shutdownNow();
-            conf.getAuthentication().close();


The reason for the leak is that when a close is abnormal, the following close will not be executed
So , we just need to ensure that these are closed normally. But now you shutdown the eventLoopGroup.

what should be done then? How does close get invoked if there occurs some exception?

@abhilashmandaliya Sorry I missed that, in some case the eventloopgroup is passed in when constructing the PulsarClient, so it's not a resource created or owned by the PulsarClient and it might be used somewhere else. In this case PulsarClient shouldn't just shut the eventloopgroup down when closing itself which might break other component using it. So we should check if the eventloopgroup is created by this PulsarClient before trying to shut it down, I think a simple boolean flag should be enough.

@MarvinCai @315157973
if we go with the boolean flag option, then the same needs to be passed to the ConnectionPool as it is also using the same eventLoopGroup. Putting a check on cnxPool.close at the caller site is not safe as ConnectionPool.close is closing some other resources as well. Adding such boolean parameter to the public constructor looks odd. So we can create a protected or package private constructor in ConnectionPool which will accept this new boolean param. Does that look good to you people or you have some other better choice here?

Discussed with Abhilash, we think it is reasonable to just close whatever resources the PulsarClientImpl create and own when constructor error out, e.g. a cnxPool or eventLoopGroup & cnxPool, and user should be responsible to close whatever resources they passed in the PulsarClientImpl's constructor when catching the exception. We will update the documentation of the API to remind user.
With this change, situation should only be better in exception case since now the PulsarClientImpl constructor is not closing any resources if there's an exception.

@MarvinCai @315157973 I have made the changes as we discussed. Also updated the java doc for the same. kindly review it and let me know if any further change is required.

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

merlimat · 2021-04-05T18:06:39Z

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/PulsarClient.java

@@ -281,7 +281,8 @@ static ClientBuilder builder() {
     * Perform immediate shutdown of PulsarClient.
     *
     * <p>Release all the resources and close all the producer, consumer and reader instances without waiting
-     * for ongoing operations to complete.
+     * for ongoing operations to complete. Resources passed from the outside like EventLoopGroup or ConnectionPool
+     * will not be closed.


EventLoopGroup and ConnectionPool classes are not part of the public API (even though they can be passed to a PulsarClientImpl constructor, which is not public), so they should not be mentioned here.

@merlimat @MarvinCai was suggesting to document it so I added it there. Any alternative or we don't need to mention that at all?

PulsarClientImpl's constructors are actually public(probably shouldn't be as we don't expect user direct create one themself?)
@abhilashmandaliya I was thinking about adding doc on PulsarClientImpl's constructor, but as it not intended to be used directly by user. And seems the ClientBuilder doesn't support pass in eventloopgroup and cnxpool for now, it doesn't make much sense to tell user about the internal of pulsarclient, we probably don't need that doc for now, and if in the future we support pass in eventloopgroup/cnxpool we can add them then.

alright. I have reverted the java doc comment. Hope tests pass this time 🤞

abhilashmandaliya · 2021-04-06T10:24:57Z

/pulsarbot run-failure-checks

abhilashmandaliya · 2021-04-06T15:16:10Z

/pulsarbot run-failure-checks

abhilashmandaliya · 2021-04-08T10:23:45Z

/pulsarbot run-failure-checks

abhilashmandaliya · 2021-04-09T15:02:11Z

/pulsarbot run-failure-checks

abhilashmandaliya · 2021-04-10T11:47:06Z

/pulsarbot run-failure-checks

abhilashmandaliya · 2021-04-10T15:14:39Z

/pulsarbot run-failure-checks

abhilashmandaliya · 2021-04-11T06:51:23Z

/pulsarbot run-failure-checks

trying to close all the resources even if previous one fails during close/shutdown code clean up as per the idea suggestions

Co-authored-by: Matteo Merli <mmerli@apache.org>

This reverts commit 3c75a2c.

Potential reason could be: multiple clients are closed and close method closes more resources now

abhilashmandaliya · 2021-04-12T08:55:13Z

/pulsarbot run-failure-checks

MarvinCai · 2021-04-12T10:13:47Z

@codelipenghui @315157973 @merlimat PTAL

eolivelli · 2021-04-29T08:10:44Z

This patch does not apply to branch-7.2

trying to close all the resources even if previous one fails during close/shutdown code clean up as per the idea suggestions Fixes apache#10010 Fixed memory leak caused by reception thrown from the PulsarClientImpl constructor in some cases. Resources were not getting closed which used to end up throwing OOM if user code tries to create a client unless succeeded. Handled exception in the constructor with try-catch and updated shutdown method to close all resources correctly. Previously, it was not attempting to close further resource if the previous one failed. Now it will try to close all of them individully. This change is a trivial rework / code cleanup without any test coverage. - Dependencies (does it add or upgrade a dependency): (yes / no): no - The public API: (yes / no): no - The schema: (yes / no / don't know): no - The default values of configurations: (yes / no): no - The wire protocol: (yes / no): no - The rest endpoints: (yes / no): no - The admin cli options: (yes / no): no - Anything that affects deployment: (yes / no / don't know): no - Does this pull request introduce a new feature? (yes / no): no - If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented): not applicable - If a feature is not applicable for documentation, explain why?: nothing to concerned with the end-user - If a feature is not documented yet in this PR, please create a followup issue for adding the documentation: not required (cherry picked from commit 197bd93)

MarvinCai reviewed Mar 24, 2021

View reviewed changes

codelipenghui reviewed Mar 24, 2021

View reviewed changes

codelipenghui approved these changes Mar 26, 2021

View reviewed changes

MarvinCai approved these changes Mar 26, 2021

View reviewed changes

315157973 reviewed Mar 26, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java Outdated Show resolved Hide resolved

abhilashmandaliya requested a review from 315157973 March 26, 2021 10:16

315157973 approved these changes Mar 26, 2021

View reviewed changes

315157973 reviewed Mar 26, 2021

View reviewed changes

merlimat reviewed Mar 29, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java Show resolved Hide resolved

sijie assigned abhilashmandaliya Mar 30, 2021

sijie added this to the 2.8.0 milestone Mar 30, 2021

sijie added area/client release/2.7.2 labels Mar 30, 2021

abhilashmandaliya force-pushed the master branch from e33793a to 8e798ae Compare April 5, 2021 16:47

abhilashmandaliya requested review from 315157973 and codelipenghui April 5, 2021 16:50

merlimat reviewed Apr 5, 2021

View reviewed changes

abhilashmandaliya requested a review from merlimat April 6, 2021 08:15

abhilashmandaliya force-pushed the master branch 2 times, most recently from 62501f5 to ce71b40 Compare April 7, 2021 13:20

abhilashmandaliya force-pushed the master branch from ce71b40 to 6844d80 Compare April 9, 2021 07:11

abhilashmandaliya force-pushed the master branch from 6844d80 to 7b8eb75 Compare April 10, 2021 09:35

abhilashmandaliya and others added 10 commits April 12, 2021 11:59

fixed memory leak mentioned in apache#10010

9e450dd

trying to close all the resources even if previous one fails during close/shutdown code clean up as per the idea suggestions

reverted licence java doc comment

d890685

added unit test for testing resource leak

7aeedd1

not closing resources if failure occurs on updating service url

6e288ce

not closing externally passed resources in pulsar client.

2a62817

replaced annotation with with any type wildcard

36f7fcf

Co-authored-by: Matteo Merli <mmerli@apache.org>

Revert "replaced annotation with with any type wildcard"

57e47c3

This reverts commit 3c75a2c.

removed extra documentation

e9a44a6

closing channels iff they are open

f6ec82c

increased time of the always failing test

4b78b94

Potential reason could be: multiple clients are closed and close method closes more resources now

abhilashmandaliya force-pushed the master branch from 7b8eb75 to 4b78b94 Compare April 12, 2021 07:29

codelipenghui approved these changes Apr 13, 2021

View reviewed changes

codelipenghui merged commit 197bd93 into apache:master Apr 13, 2021

eolivelli removed the release/2.7.2 label Apr 29, 2021

merlimat mentioned this pull request Aug 30, 2021

Fixed Proxy leaking oubound connections #11848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 10010][Client] fixed memory leak #10028

[Issue 10010][Client] fixed memory leak #10028

abhilashmandaliya commented Mar 24, 2021

MarvinCai left a comment

MarvinCai Mar 24, 2021

abhilashmandaliya Mar 24, 2021

MarvinCai Mar 24, 2021

abhilashmandaliya Mar 24, 2021

codelipenghui left a comment

abhilashmandaliya commented Mar 25, 2021

codelipenghui commented Mar 26, 2021

MarvinCai commented Mar 26, 2021

315157973 left a comment

abhilashmandaliya commented Mar 26, 2021 •

edited

Loading

315157973 Mar 26, 2021

abhilashmandaliya Mar 26, 2021

MarvinCai Mar 26, 2021 •

edited

Loading

abhilashmandaliya Mar 26, 2021

MarvinCai Mar 31, 2021

abhilashmandaliya Apr 5, 2021

merlimat Apr 5, 2021

abhilashmandaliya Apr 6, 2021

MarvinCai Apr 6, 2021

abhilashmandaliya Apr 6, 2021

abhilashmandaliya commented Apr 6, 2021

abhilashmandaliya commented Apr 6, 2021

abhilashmandaliya commented Apr 8, 2021

abhilashmandaliya commented Apr 9, 2021

abhilashmandaliya commented Apr 10, 2021

abhilashmandaliya commented Apr 10, 2021

abhilashmandaliya commented Apr 11, 2021

abhilashmandaliya commented Apr 12, 2021

MarvinCai commented Apr 12, 2021

eolivelli commented Apr 29, 2021

[Issue 10010][Client] fixed memory leak #10028

[Issue 10010][Client] fixed memory leak #10028

Conversation

abhilashmandaliya commented Mar 24, 2021

Motivation

Modifications

Does this pull request potentially affect one of the following parts:

Documentation

MarvinCai left a 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

codelipenghui left a comment

Choose a reason for hiding this comment

abhilashmandaliya commented Mar 25, 2021

codelipenghui commented Mar 26, 2021

MarvinCai commented Mar 26, 2021

315157973 left a comment

Choose a reason for hiding this comment

abhilashmandaliya commented Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarvinCai Mar 26, 2021 • edited Loading

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

Choose a reason for hiding this comment

abhilashmandaliya commented Apr 6, 2021

abhilashmandaliya commented Apr 6, 2021

abhilashmandaliya commented Apr 8, 2021

abhilashmandaliya commented Apr 9, 2021

abhilashmandaliya commented Apr 10, 2021

abhilashmandaliya commented Apr 10, 2021

abhilashmandaliya commented Apr 11, 2021

abhilashmandaliya commented Apr 12, 2021

MarvinCai commented Apr 12, 2021

eolivelli commented Apr 29, 2021

abhilashmandaliya commented Mar 26, 2021 •

edited

Loading

MarvinCai Mar 26, 2021 •

edited

Loading