fixed NPE in pulsar client #10470
fixed NPE in pulsar client #10470
Conversation
… conf.getServiceUrl is blank
|
@merlimat @codelipenghui PTAL |
| } | ||
| try { | ||
| closeCnxPool(cnxPool); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
what about catching PulsarClientException and not a raw Throwable ?
There was a problem hiding this comment.
okay. should I change it everywhere in this function? @eolivelli
There was a problem hiding this 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.
There was a problem hiding this comment.
alright. thanks. I have changed it
There was a problem hiding this 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,
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
* fixed NPE from pulsar client if we attempt to close the resource when conf.getServiceUrl is blank * removed redundant check * catching PulsarClientException instead of Throwable * catching PulsarClientException instead of Throwable
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.