You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "gabriele renzi (JIRA)" <ji...@apache.org> on 2010/02/24 00:06:27 UTC
[jira] Created: (CASSANDRA-828) possible NPE in StorageService
possible NPE in StorageService
------------------------------
Key: CASSANDRA-828
URL: https://issues.apache.org/jira/browse/CASSANDRA-828
Project: Cassandra
Issue Type: Bug
Affects Versions: 0.6
Reporter: gabriele renzi
Priority: Minor
the code
{{{
if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
{
logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
return;
}
if (logger_.isDebugEnabled())
logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
if (endPointThatLeft != null)
{
removeEndPointLocally(endPointThatLeft);
}
}}}
appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (CASSANDRA-828) possible NPE in StorageService
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CASSANDRA-828?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Ellis resolved CASSANDRA-828.
--------------------------------------
Resolution: Fixed
Fix Version/s: 0.6
Assignee: gabriele renzi
> if it is possible for the leaving endpoint to be null
it's not
> getLocalAddress should probably be synchronized, or localInetAddress made volatile
made localInetAddress volatile in r915580, thanks.
> possible NPE in StorageService
> ------------------------------
>
> Key: CASSANDRA-828
> URL: https://issues.apache.org/jira/browse/CASSANDRA-828
> Project: Cassandra
> Issue Type: Bug
> Affects Versions: 0.6
> Reporter: gabriele renzi
> Assignee: gabriele renzi
> Priority: Minor
> Fix For: 0.6
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> the code
> {{{
> if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
> {
> logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
> return;
> }
> if (logger_.isDebugEnabled())
> logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
> if (endPointThatLeft != null)
> {
> removeEndPointLocally(endPointThatLeft);
> }
> }}}
> appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
> As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (CASSANDRA-828) possible NPE in StorageService
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CASSANDRA-828?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838033#action_12838033 ]
Jonathan Ellis commented on CASSANDRA-828:
------------------------------------------
my mistake: endpoint, the argument to the method, can't be null, but endpointThatLeft can be. Made suggested fix of inverting if condition in r916008.
> possible NPE in StorageService
> ------------------------------
>
> Key: CASSANDRA-828
> URL: https://issues.apache.org/jira/browse/CASSANDRA-828
> Project: Cassandra
> Issue Type: Bug
> Affects Versions: 0.6
> Reporter: gabriele renzi
> Assignee: gabriele renzi
> Priority: Minor
> Fix For: 0.6
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> the code
> {{{
> if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
> {
> logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
> return;
> }
> if (logger_.isDebugEnabled())
> logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
> if (endPointThatLeft != null)
> {
> removeEndPointLocally(endPointThatLeft);
> }
> }}}
> appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
> As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (CASSANDRA-828) possible NPE in StorageService
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CASSANDRA-828?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Ellis resolved CASSANDRA-828.
--------------------------------------
Resolution: Fixed
the != null check is definitely redundant. removed in r916006.
> possible NPE in StorageService
> ------------------------------
>
> Key: CASSANDRA-828
> URL: https://issues.apache.org/jira/browse/CASSANDRA-828
> Project: Cassandra
> Issue Type: Bug
> Affects Versions: 0.6
> Reporter: gabriele renzi
> Assignee: gabriele renzi
> Priority: Minor
> Fix For: 0.6
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> the code
> {{{
> if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
> {
> logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
> return;
> }
> if (logger_.isDebugEnabled())
> logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
> if (endPointThatLeft != null)
> {
> removeEndPointLocally(endPointThatLeft);
> }
> }}}
> appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
> As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Reopened: (CASSANDRA-828) possible NPE in StorageService
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CASSANDRA-828?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Ellis reopened CASSANDRA-828:
--------------------------------------
you're right, something is fishy here. reopened.
> possible NPE in StorageService
> ------------------------------
>
> Key: CASSANDRA-828
> URL: https://issues.apache.org/jira/browse/CASSANDRA-828
> Project: Cassandra
> Issue Type: Bug
> Affects Versions: 0.6
> Reporter: gabriele renzi
> Assignee: gabriele renzi
> Priority: Minor
> Fix For: 0.6
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> the code
> {{{
> if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
> {
> logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
> return;
> }
> if (logger_.isDebugEnabled())
> logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
> if (endPointThatLeft != null)
> {
> removeEndPointLocally(endPointThatLeft);
> }
> }}}
> appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
> As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (CASSANDRA-828) possible NPE in StorageService
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CASSANDRA-828?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Ellis updated CASSANDRA-828:
-------------------------------------
Component/s: Core
Affects Version/s: (was: 0.6)
0.5
> possible NPE in StorageService
> ------------------------------
>
> Key: CASSANDRA-828
> URL: https://issues.apache.org/jira/browse/CASSANDRA-828
> Project: Cassandra
> Issue Type: Bug
> Components: Core
> Affects Versions: 0.5
> Reporter: gabriele renzi
> Assignee: gabriele renzi
> Priority: Minor
> Fix For: 0.6
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> the code
> {{{
> if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
> {
> logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
> return;
> }
> if (logger_.isDebugEnabled())
> logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
> if (endPointThatLeft != null)
> {
> removeEndPointLocally(endPointThatLeft);
> }
> }}}
> appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
> As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (CASSANDRA-828) possible NPE in StorageService
Posted by "gabriele renzi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CASSANDRA-828?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12837511#action_12837511 ]
gabriele renzi commented on CASSANDRA-828:
------------------------------------------
wow, this was fast, thanks.
But then aren't the following two checks for nullness unnecessary?
> possible NPE in StorageService
> ------------------------------
>
> Key: CASSANDRA-828
> URL: https://issues.apache.org/jira/browse/CASSANDRA-828
> Project: Cassandra
> Issue Type: Bug
> Affects Versions: 0.6
> Reporter: gabriele renzi
> Assignee: gabriele renzi
> Priority: Minor
> Fix For: 0.6
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> the code
> {{{
> if (endPointThatLeft.equals(FBUtilities.getLocalAddress()))
> {
> logger_.info("Received removeToken gossip about myself. Is this node a replacement for a removed one?");
> return;
> }
> if (logger_.isDebugEnabled())
> logger_.debug("Token " + token + " removed manually (endpoint was " + ((endPointThatLeft == null) ? "unknown" : endPointThatLeft) + ")");
> if (endPointThatLeft != null)
> {
> removeEndPointLocally(endPointThatLeft);
> }
> }}}
> appears wrong: if it is possible for the leaving endpoint to be unknown then the first "if" has a possible null dereference, which can be eliminated by swapping the arguments or reordering the code.
> As a side note, I believe FBUtilities.getLocalAddress should probably be synchronized (or localInetAddress made volatile) per the usual "the java MM does not guarantee any change will ever be visible" mantra which may or may not be considered relevant :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.