You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/05/03 07:13:28 UTC
[GitHub] [pulsar] abhilashmandaliya opened a new pull request #10470: fixed NPE in pulsar client
abhilashmandaliya opened a new pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470
### Motivation
*PulsarClientImpl throws NPE if we attempt to close the resource when conf.getServiceUrl is blank.*
### Modifications
*Added a null check before accessing the resource.*
This change is a trivial rework / code cleanup without any test coverage.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#discussion_r624970070
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
##########
@@ -718,13 +719,11 @@ public void shutdown() throws PulsarClientException {
log.warn("Failed to shutdown eventLoopGroup", t);
throwable = t;
}
- if (createdCnxPool) {
- try {
- cnxPool.close();
- } catch (Throwable t) {
- log.warn("Failed to shutdown cnxPool", t);
- throwable = t;
- }
+ try {
+ closeCnxPool(cnxPool);
+ } catch (Throwable t) {
Review comment:
@eolivelli I changed at 2 other places as well in this function and I believe it is safe as I had done those changes a month ago and we haven't released after that,
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#discussion_r624970070
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
##########
@@ -718,13 +719,11 @@ public void shutdown() throws PulsarClientException {
log.warn("Failed to shutdown eventLoopGroup", t);
throwable = t;
}
- if (createdCnxPool) {
- try {
- cnxPool.close();
- } catch (Throwable t) {
- log.warn("Failed to shutdown cnxPool", t);
- throwable = t;
- }
+ try {
+ closeCnxPool(cnxPool);
+ } catch (Throwable t) {
Review comment:
@eolivelli I changed at 2 other places in this function, which call some local private functions and they do throw PulsarClientException.
I believe it is safe as I had done those changes a month ago and we haven't released after that,
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#discussion_r624936476
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
##########
@@ -718,13 +719,11 @@ public void shutdown() throws PulsarClientException {
log.warn("Failed to shutdown eventLoopGroup", t);
throwable = t;
}
- if (createdCnxPool) {
- try {
- cnxPool.close();
- } catch (Throwable t) {
- log.warn("Failed to shutdown cnxPool", t);
- throwable = t;
- }
+ try {
+ closeCnxPool(cnxPool);
+ } catch (Throwable t) {
Review comment:
alright. thanks. I have changed it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#discussion_r624921036
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
##########
@@ -718,13 +719,11 @@ public void shutdown() throws PulsarClientException {
log.warn("Failed to shutdown eventLoopGroup", t);
throwable = t;
}
- if (createdCnxPool) {
- try {
- cnxPool.close();
- } catch (Throwable t) {
- log.warn("Failed to shutdown cnxPool", t);
- throwable = t;
- }
+ try {
+ closeCnxPool(cnxPool);
+ } catch (Throwable t) {
Review comment:
okay. should I change it everywhere in this function? @eolivelli
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#issuecomment-831198288
/pulsarbot run-failure-checks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#discussion_r624920174
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
##########
@@ -718,13 +719,11 @@ public void shutdown() throws PulsarClientException {
log.warn("Failed to shutdown eventLoopGroup", t);
throwable = t;
}
- if (createdCnxPool) {
- try {
- cnxPool.close();
- } catch (Throwable t) {
- log.warn("Failed to shutdown cnxPool", t);
- throwable = t;
- }
+ try {
+ closeCnxPool(cnxPool);
+ } catch (Throwable t) {
Review comment:
what about catching PulsarClientException and not a raw Throwable ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#issuecomment-831070974
@merlimat @codelipenghui PTAL
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#discussion_r624924112
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
##########
@@ -718,13 +719,11 @@ public void shutdown() throws PulsarClientException {
log.warn("Failed to shutdown eventLoopGroup", t);
throwable = t;
}
- if (createdCnxPool) {
- try {
- cnxPool.close();
- } catch (Throwable t) {
- log.warn("Failed to shutdown cnxPool", t);
- throwable = t;
- }
+ try {
+ closeCnxPool(cnxPool);
+ } catch (Throwable t) {
Review comment:
I won't change other points, the effect may be unpredictable.
in your new function `closeCnxPool` you are already catching Throwable so here it is useless.
catching `Throwable` and `Exception` is always a code smell and we should do it only in very specific cases when we know what we are doing.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10470: fixed NPE in pulsar client
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10470:
URL: https://github.com/apache/pulsar/pull/10470#issuecomment-831186123
/pulsarbot run-failure-checks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org