Solr
  1. Solr
  2. SOLR-8254

HttpSolrCall.getCoreByCollection can throw NPE if there are no leaders up

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.4
    • Component/s: None
    • Labels:
      None

      Activity

      Hide
      Alan Woodward added a comment -

      Lines 781-794:

          //Hitting the leaders is useful when it's an update request.
          //For queries it doesn't matter and hence we don't distinguish here.
          for (Map.Entry<String, Slice> entry : entries) {
            // first see if we have the leader
            Replica leaderProps = clusterState.getLeader(collection, entry.getKey());    // <----- CAN RETURN NULL IF THERE'S NO LEADER
            if (liveNodes.contains(leaderProps.getNodeName()) && leaderProps.getState() == Replica.State.ACTIVE) {
              if (leaderProps != null) {
                core = checkProps(leaderProps);
              }
              if (core != null) {
                return core;
              }
            }
      

      The leaderProps null check should be part of the outer if statement, I think, but I don't grok this bit of code well enough to know what to do if the leader is in fact null.

      It also strikes me that this method probably belongs on CoreContainer, rather than as a private method in HttpSolrCall?

      Show
      Alan Woodward added a comment - Lines 781-794: //Hitting the leaders is useful when it's an update request. //For queries it doesn't matter and hence we don't distinguish here. for (Map.Entry< String , Slice> entry : entries) { // first see if we have the leader Replica leaderProps = clusterState.getLeader(collection, entry.getKey()); // <----- CAN RETURN NULL IF THERE'S NO LEADER if (liveNodes.contains(leaderProps.getNodeName()) && leaderProps.getState() == Replica.State.ACTIVE) { if (leaderProps != null ) { core = checkProps(leaderProps); } if (core != null ) { return core; } } The leaderProps null check should be part of the outer if statement, I think, but I don't grok this bit of code well enough to know what to do if the leader is in fact null. It also strikes me that this method probably belongs on CoreContainer, rather than as a private method in HttpSolrCall?
      Hide
      Ishan Chattopadhyaya added a comment -

      The leaderProps null check should be part of the outer if statement

      Can static analysis of code help here uncover such bugs? I mean tools like FindBugs or the one from JetBrains etc.?

      Show
      Ishan Chattopadhyaya added a comment - The leaderProps null check should be part of the outer if statement Can static analysis of code help here uncover such bugs? I mean tools like FindBugs or the one from JetBrains etc.?
      Hide
      Mark Miller added a comment -

      +1, I hit this working on SOLR-6237 as well. Same fix.

      Index: solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
      ===================================================================
      --- solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java	(revision 1712839)
      +++ solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java	(working copy)
      @@ -783,12 +783,12 @@
           for (Map.Entry<String, Slice> entry : entries) {
             // first see if we have the leader
             Replica leaderProps = clusterState.getLeader(collection, entry.getKey());
      -      if (liveNodes.contains(leaderProps.getNodeName()) && leaderProps.getState() == Replica.State.ACTIVE) {
      -        if (leaderProps != null) {
      +      if (leaderProps != null) {
      +        if (liveNodes.contains(leaderProps.getNodeName()) && leaderProps.getState() == Replica.State.ACTIVE) {
                 core = checkProps(leaderProps);
      -        }
      -        if (core != null) {
      -          return core;
      +          if (core != null) {
      +            return core;
      +          }
               }
             }
      

      It also strikes me that this method probably belongs on CoreContainer, rather than as a private method in HttpSolrCall?

      I don't know, we do not tend to put a lot of SolrCloud specific methods on CoreContainer.

      Show
      Mark Miller added a comment - +1, I hit this working on SOLR-6237 as well. Same fix. Index: solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java =================================================================== --- solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java (revision 1712839) +++ solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java (working copy) @@ -783,12 +783,12 @@ for (Map.Entry<String, Slice> entry : entries) { // first see if we have the leader Replica leaderProps = clusterState.getLeader(collection, entry.getKey()); - if (liveNodes.contains(leaderProps.getNodeName()) && leaderProps.getState() == Replica.State.ACTIVE) { - if (leaderProps != null) { + if (leaderProps != null) { + if (liveNodes.contains(leaderProps.getNodeName()) && leaderProps.getState() == Replica.State.ACTIVE) { core = checkProps(leaderProps); - } - if (core != null) { - return core; + if (core != null) { + return core; + } } } It also strikes me that this method probably belongs on CoreContainer, rather than as a private method in HttpSolrCall? I don't know, we do not tend to put a lot of SolrCloud specific methods on CoreContainer.
      Hide
      Alan Woodward added a comment -

      I don't know, we do not tend to put a lot of SolrCloud specific methods on CoreContainer.

      Fair enough, I'll leave it where it is.

      Can static analysis of code help here uncover such bugs?

      Almost certainly - IntelliJ showed me the error immediately when I looked at the code. I have a feeling that this has come up before, but getting the whole codebase to pass will be a pretty big job.

      Show
      Alan Woodward added a comment - I don't know, we do not tend to put a lot of SolrCloud specific methods on CoreContainer. Fair enough, I'll leave it where it is. Can static analysis of code help here uncover such bugs? Almost certainly - IntelliJ showed me the error immediately when I looked at the code. I have a feeling that this has come up before, but getting the whole codebase to pass will be a pretty big job.
      Hide
      ASF subversion and git services added a comment -

      Commit 1713468 from Alan Woodward in branch 'dev/trunk'
      [ https://svn.apache.org/r1713468 ]

      SOLR-8254: HttpSolrCore.getCoreByCollection() can throw NPE

      Show
      ASF subversion and git services added a comment - Commit 1713468 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1713468 ] SOLR-8254 : HttpSolrCore.getCoreByCollection() can throw NPE
      Hide
      ASF subversion and git services added a comment -

      Commit 1713471 from Alan Woodward in branch 'dev/branches/branch_5x'
      [ https://svn.apache.org/r1713471 ]

      SOLR-8254: HttpSolrCore.getCoreByCollection() can throw NPE

      Show
      ASF subversion and git services added a comment - Commit 1713471 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1713471 ] SOLR-8254 : HttpSolrCore.getCoreByCollection() can throw NPE

        People

        • Assignee:
          Alan Woodward
          Reporter:
          Alan Woodward
        • Votes:
          0 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development