You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jostbg <gi...@git.apache.org> on 2018/01/11 11:17:16 UTC
[GitHub] activemq-artemis pull request #1772: ARTEMIS-1601 Prevent NPE in StompSessio...
GitHub user jostbg opened a pull request:
https://github.com/apache/activemq-artemis/pull/1772
ARTEMIS-1601 Prevent NPE in StompSession#sendMessage
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jostbg/activemq-artemis ARTEMIS-1601
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1772.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1772
----
commit 185983d4dff8e9d256fc65cb2e1fe0e7b12ec8fb
Author: jostbg <35...@...>
Date: 2018-01-11T11:16:35Z
ARTEMIS-1601 Prevent NPE in StompSession#sendMessage
----
---
[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1772
+1 I think It’s cleaner than relying exception to occur
---
[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1772
IMO, it's less clean because now there are 2 places which are returning 0 in case of an error. In any case, it's really not a big deal.
One additional question here...Is this NPE mainly theoretical or is there a specific use-case which triggers it? If the former then I'm even less in favor of merging the PR. If the latter then it would be worth adding a test or maybe just describing the use-case in the commit message.
---
[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...
Posted by jostbg <gi...@git.apache.org>.
Github user jostbg commented on the issue:
https://github.com/apache/activemq-artemis/pull/1772
The NPEs occur that's why I opened this PR. Since they are only logged in debug level I only saw them while debugging some stomp issue I had.
Throwing an exception is a relatively expensive operation in the JVM because of the stacktrace information that needs to be generated, so in cases where it is know that a null value could be returned it one should not rely on an NPE to be catched.
---
[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1772
I'll buy that.
---
[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...
Posted by jostbg <gi...@git.apache.org>.
Github user jostbg commented on the issue:
https://github.com/apache/activemq-artemis/pull/1772
I wonder if that may be a race condition between StompSession#disconnect and StompSession#sendMessage, i.e. if a disconnect happens while sendMessage is being executed or something.
---
[GitHub] activemq-artemis pull request #1772: ARTEMIS-1601 Prevent NPE in StompSessio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1772
---
[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1772
I'm not sure this is actually necessary. `java.lang.NullPointerException` extends `java.lang.RuntimeException` which in turn extends `java.lang.Exception` which is already being caught for this `try` block and returning 0.
---