Solr
  1. Solr
  2. SOLR-3084

default="true" causes NPE -> SolrException from QueryResponseWriter initWriters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: Response Writers
    • Labels:
      None

      Description

      This is from issue SOLR-2718 and came up after backporting that issue to branch_3x.
      After that backport I get a SolrException.

      Jan 31, 2012 1:50:15 PM org.apache.solr.core.SolrCore initListeners
      INFO: [] Added SolrEventListener for firstSearcher: org.apache.solr.core.QuerySenderListener{queries=[

      {q=*:*,start=0,rows=10,spellcheck.build=true}

      , .....
      Jan 31, 2012 2:00:10 PM org.apache.solr.common.SolrException log
      SEVERE: org.apache.solr.common.SolrException: QueryResponseWriter init failure
      at org.apache.solr.core.SolrCore.initWriters(SolrCore.java:1499)
      at org.apache.solr.core.SolrCore.<init>(SolrCore.java:557)
      at org.apache.solr.core.CoreContainer.create(CoreContainer.java:466)
      at org.apache.solr.core.CoreContainer.load(CoreContainer.java:319)
      ...

      1. SOLR-3084.patch
        2 kB
        Hoss Man
      2. SOLR-3084.patch
        0.8 kB
        Bernd Fehling

        Activity

        Hide
        Bernd Fehling added a comment -

        It turned out that log.warn is always triggered, also for the first default queryResponseWriter.

        if(info.isDefault()){
          defaultResponseWriter = writer;
          if(defaultResponseWriter != null)
            log.warn("Multiple default queryResponseWriter registered ignoring: " + old.getClass().getName());
        }
        

        Solution is to place "defaultResponseWriter = writer;" after the if clause.

        if(info.isDefault()){
          if(defaultResponseWriter != null)
            log.warn("Multiple default queryResponseWriter registered ignoring: " + old.getClass().getName());
          defaultResponseWriter = writer;
        }
        
        Show
        Bernd Fehling added a comment - It turned out that log.warn is always triggered, also for the first default queryResponseWriter. if(info.isDefault()){ defaultResponseWriter = writer; if(defaultResponseWriter != null) log.warn( "Multiple default queryResponseWriter registered ignoring: " + old.getClass().getName()); } Solution is to place "defaultResponseWriter = writer;" after the if clause. if(info.isDefault()){ if(defaultResponseWriter != null) log.warn( "Multiple default queryResponseWriter registered ignoring: " + old.getClass().getName()); defaultResponseWriter = writer; }
        Hide
        Hoss Man added a comment -

        Bernd: I'm not sure why you resolved this? it definitely looks like a very annoying and serious bug that would burn anyone overriding the "default" responseWriter – and not even in a way that would give them a useful error message

        the problem isn't just the order of the assignment as you fix in your patch, but also that:

        • this check assumes "old" is non-null, which this check has no buisness assuming
        • that "old" is in some way a reflection of the duplicate "defaults" ... which is completely un-true.
        Show
        Hoss Man added a comment - Bernd: I'm not sure why you resolved this? it definitely looks like a very annoying and serious bug that would burn anyone overriding the "default" responseWriter – and not even in a way that would give them a useful error message the problem isn't just the order of the assignment as you fix in your patch, but also that: this check assumes "old" is non-null, which this check has no buisness assuming that "old" is in some way a reflection of the duplicate "defaults" ... which is completely un-true.
        Hide
        Hoss Man added a comment -

        Updated patch to fix the check to not use "old" at all.

        also tweaked a test config so that 'default="true"' was used, making this failure extremely evident in tests (w/o this patch)

        Show
        Hoss Man added a comment - Updated patch to fix the check to not use "old" at all. also tweaked a test config so that 'default="true"' was used, making this failure extremely evident in tests (w/o this patch)
        Hide
        Hoss Man added a comment -

        Thanks for reporting this and helping get to the bottom of it Bernd!

        Committed revision 1239316. - trunk
        Committed revision 1239327. - 3x

        Show
        Hoss Man added a comment - Thanks for reporting this and helping get to the bottom of it Bernd! Committed revision 1239316. - trunk Committed revision 1239327. - 3x
        Hide
        Bernd Fehling added a comment -

        Thanks a lot for checking my patch.

        Show
        Bernd Fehling added a comment - Thanks a lot for checking my patch.

          People

          • Assignee:
            Bernd Fehling
            Reporter:
            Bernd Fehling
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development