Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8191

Guard against CloudSolrStream close method NullPointerException

    Details

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

      Description

      CloudSolrStream doesn't check if cloudSolrClient or solrStreams is null yielding a NullPointerException in those cases when close() is called on it.

      1. SOLR-8191.patch
        1 kB
        Kevin Risden

        Issue Links

          Activity

          Hide
          gerlowskija Jason Gerlowski added a comment -

          The current patch still applies cleanly. Fixing this NPE might not be hugely important, but this bug is blocking SOLR-8190, which would be a nice improvement IMO. (Not a huge deal, but still a nice little tidbit).

          Looking at CloudSolrStream a little closer though, it seems odd to perform a null-check on close(), but not any of the other places that cloudSolrClient is used. For instance, check out the protected constructStreams() method, which is invoked on each call to open().

          Those are just my observations at a glance. I'm not very familiar with the SolrJ code, so maybe this isn't actually an issue. Just wanted to mention it. I'm going to tinker around with this more tonight to see if I can learn more.

          Show
          gerlowskija Jason Gerlowski added a comment - The current patch still applies cleanly. Fixing this NPE might not be hugely important, but this bug is blocking SOLR-8190 , which would be a nice improvement IMO. (Not a huge deal, but still a nice little tidbit). Looking at CloudSolrStream a little closer though, it seems odd to perform a null-check on close() , but not any of the other places that cloudSolrClient is used. For instance, check out the protected constructStreams() method, which is invoked on each call to open() . Those are just my observations at a glance. I'm not very familiar with the SolrJ code, so maybe this isn't actually an issue. Just wanted to mention it. I'm going to tinker around with this more tonight to see if I can learn more.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          After a little more attention, I think it'd probably be safer to null-check in constructStreams as well (see above). That said, I'm not sure if there was a rationale for avoiding this so far. Kevin Risden, was there a reason you hadn't done this in your initial patch?

          In any case, the patch does fix the NPE's exposed by closing the streams in the Streaming tests, so in that respect it looks good to me.

          Can someone with more familiarity with this part of the codebase (ideally someone willing to consider merging this) take a look at this patch please

          Show
          gerlowskija Jason Gerlowski added a comment - After a little more attention, I think it'd probably be safer to null-check in constructStreams as well (see above). That said, I'm not sure if there was a rationale for avoiding this so far. Kevin Risden , was there a reason you hadn't done this in your initial patch? In any case, the patch does fix the NPE's exposed by closing the streams in the Streaming tests, so in that respect it looks good to me. Can someone with more familiarity with this part of the codebase (ideally someone willing to consider merging this) take a look at this patch please
          Hide
          joel.bernstein Joel Bernstein added a comment - - edited

          Sure, I should have some time to review this ticket and SOLR-8190 tomorrow.

          Show
          joel.bernstein Joel Bernstein added a comment - - edited Sure, I should have some time to review this ticket and SOLR-8190 tomorrow.
          Hide
          risdenk Kevin Risden added a comment -

          Jason Gerlowski I hadn't looked for other places to do null-checks other than what was found by SOLR-8190. As you said there are probably other places that need hardening.

          Show
          risdenk Kevin Risden added a comment - Jason Gerlowski I hadn't looked for other places to do null-checks other than what was found by SOLR-8190 . As you said there are probably other places that need hardening.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          Ok, this looks fine to me.

          Show
          joel.bernstein Joel Bernstein added a comment - Ok, this looks fine to me.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1720460 from Joel Bernstein in branch 'dev/trunk'
          [ https://svn.apache.org/r1720460 ]

          SOLR-8191: Gaurd against CloudSolrStream close method NullPointerException

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1720460 from Joel Bernstein in branch 'dev/trunk' [ https://svn.apache.org/r1720460 ] SOLR-8191 : Gaurd against CloudSolrStream close method NullPointerException
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1720461 from Joel Bernstein in branch 'dev/trunk'
          [ https://svn.apache.org/r1720461 ]

          SOLR-8191: Update CHANGES.txt

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1720461 from Joel Bernstein in branch 'dev/trunk' [ https://svn.apache.org/r1720461 ] SOLR-8191 : Update CHANGES.txt

            People

            • Assignee:
              Unassigned
              Reporter:
              risdenk Kevin Risden
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development