Re: [PR] [fix] [client] Fix memory leak when using API pulsarClient.getLookup(url) [pulsar]
poorbarcode commented on code in PR #22858:
URL: https://github.com/apache/pulsar/pull/22858#discussion_r1629184307
##
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java:
##
@@ -982,6 +1000,10 @@ public CompletableFuture getConnection(final
String topic, final Stri
public LookupService getLookup(String serviceUrl) {
return urlLookupMap.computeIfAbsent(serviceUrl, url -> {
+if (isClosed()) {
+throw new IllegalStateException("Pulsar client has been
closed, can not build LookupService when"
++ " calling get lookup with an url");
+}
Review Comment:
> I don't think it's actual a memory leak. This PR only clears the map after
closeAsync is called. However, I could hardly think of a case that a
PulsarClient is still used after closeAsync. When the PulsarClientImpl object
is GC'ed, this map will be GC'ed as well.
- The map obj is not an important point.
- `LookupService` is a child class of `Closable`, which means it always
holds resources (such as connections or other resources). Before lookup
services have been GC, should call `close`.
> Move it before the `computeIfAbsent`.
In the method `computeIfAbsent` is better, because there is a case like
below:
| `thread-1`| `thread-2` |
| --- | --- |
| | check state: `open` |
| check state -> closing |
| for-each: close |
| | `computeIfAbsent` |
The new lookup service created through `thread-2` will not be released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [client] Fix memory leak when using API pulsarClient.getLookup(url) [pulsar]
poorbarcode commented on code in PR #22858:
URL: https://github.com/apache/pulsar/pull/22858#discussion_r1629184307
##
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java:
##
@@ -982,6 +1000,10 @@ public CompletableFuture getConnection(final
String topic, final Stri
public LookupService getLookup(String serviceUrl) {
return urlLookupMap.computeIfAbsent(serviceUrl, url -> {
+if (isClosed()) {
+throw new IllegalStateException("Pulsar client has been
closed, can not build LookupService when"
++ " calling get lookup with an url");
+}
Review Comment:
> I don't think it's actual a memory leak. This PR only clears the map after
closeAsync is called. However, I could hardly think of a case that a
PulsarClient is still used after closeAsync. When the PulsarClientImpl object
is GC'ed, this map will be GC'ed as well.
- The map obj is not an important point.
- `LookupService` is a child class of `Closable`, which means it always
holds resources (such as connections or other resources). Before lookup
services have been GC, should call `close`.
> Move it before the `computeIfAbsent`.
In the method `computeIfAbsent` is better, because there is a case like
below:
| `thread-1`| `thread-2` |
| --- | --- |
| | check state: `open` |
| check state -> closing |
| for-each: close |
| | computeIfAbsent |
The new lookup service created through `thread-2` will not be released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [client] Fix memory leak when using API pulsarClient.getLookup(url) [pulsar]
poorbarcode commented on code in PR #22858:
URL: https://github.com/apache/pulsar/pull/22858#discussion_r1629184307
##
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java:
##
@@ -982,6 +1000,10 @@ public CompletableFuture getConnection(final
String topic, final Stri
public LookupService getLookup(String serviceUrl) {
return urlLookupMap.computeIfAbsent(serviceUrl, url -> {
+if (isClosed()) {
+throw new IllegalStateException("Pulsar client has been
closed, can not build LookupService when"
++ " calling get lookup with an url");
+}
Review Comment:
> I don't think it's actual a memory leak. This PR only clears the map after
closeAsync is called. However, I could hardly think of a case that a
PulsarClient is still used after closeAsync. When the PulsarClientImpl object
is GC'ed, this map will be GC'ed as well.
- The map obj is not an important point.
- `LookupService` is a child class of `Closable`, which means it always
holds resources (such as connections or other resources). Before lookup
services have been GC, should call `close`.
> Move it before the `computeIfAbsent`.
In the method `computeIfAbsent` is better, because there is a case like
below:
| `thread-1`| `thread-2` |
| --- | --- |
| for-each: close |
| | computeIfAbsent |
The new lookup service created through `thread-2` will not be released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [client] Fix memory leak when using API pulsarClient.getLookup(url) [pulsar]
BewareMyPower commented on code in PR #22858:
URL: https://github.com/apache/pulsar/pull/22858#discussion_r1629166558
##
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java:
##
@@ -982,6 +1000,10 @@ public CompletableFuture getConnection(final
String topic, final Stri
public LookupService getLookup(String serviceUrl) {
return urlLookupMap.computeIfAbsent(serviceUrl, url -> {
+if (isClosed()) {
+throw new IllegalStateException("Pulsar client has been
closed, can not build LookupService when"
++ " calling get lookup with an url");
+}
Review Comment:
Move it before the `computeIfAbsent`.
Actually it should be an internal API. If users don't call it by cast a
`PulsarClient` object to `PulsarClientImpl`, this case should not happen.
A better solution is to make it package visible, as well as the
`getConnection` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
