[GitHub] activemq-artemis pull request #1772: ARTEMIS-1601 Prevent NPE in StompSessio...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1772: ARTEMIS-1601 Prevent NPE in StompSessio...

RaiSaurabh
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 <35264802+jostbg@...>
Date:   2018-01-11T11:16:35Z

    ARTEMIS-1601 Prevent NPE in StompSession#sendMessage

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...

RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...

RaiSaurabh
In reply to this post by RaiSaurabh
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...

RaiSaurabh
In reply to this post by RaiSaurabh
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1772: ARTEMIS-1601 Prevent NPE in StompSession#sendM...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1772
 
    I'll buy that.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1772: ARTEMIS-1601 Prevent NPE in StompSessio...

RaiSaurabh
In reply to this post by RaiSaurabh
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1772


---