Lucene - Core
  1. Lucene - Core
  2. LUCENE-4796

NamedSPILoader.reload needs to be synchronized

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spun off of SOLR-4373: as discsused with uwe on IRC, NamedSPILoader.reload is not thread safe: it reads from this.services at the beginging of hte method, makes additions based on the method input, and then overwrites this.services at the end of the method. if the method is called by two threads concurrently, the entries added by threadB could be lost if threadA enters the method before threadB and exists the method after threadB

      1. LUCENE-4796.patch
        2 kB
        Uwe Schindler
      2. LUCENE-4796.patch
        2 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          Hoss: I agree!

          The concurrency issue in NamedSPILoader can indeed be solved by making the reload method synchonized. The services field is volatile, so readers will in any case see the correct value, otherwise all methods would need to be synchronized. By using a voltile, we only need to synchronize this single method.

          Show
          Uwe Schindler added a comment - Hoss: I agree! The concurrency issue in NamedSPILoader can indeed be solved by making the reload method synchonized. The services field is volatile, so readers will in any case see the correct value, otherwise all methods would need to be synchronized. By using a voltile, we only need to synchronize this single method.
          Hide
          Uwe Schindler added a comment -

          We have to fix the same issue in AnalysisSPILoader which is (unfortunately) a different class with some code duplication (in analysis/common module).

          Show
          Uwe Schindler added a comment - We have to fix the same issue in AnalysisSPILoader which is (unfortunately) a different class with some code duplication (in analysis/common module).
          Hide
          Hoss Man added a comment -

          We have to fix the same issue in AnalysisSPILoader which is (unfortunately) a different class with some code duplication

          Ahhhh... that could totally explain why my naive attempt at fixing SOLR-4373 a while back didn't seem to work – i was only aware of NamedSPILoader but did ad hock testing using analyzer factories.

          Uwe: looking at your patch, one thing that jumps out at me is that AnalysisSPILoader seems to have another exist bug that may also cause some similar problems, regardless of thread safety...

            public synchronized void reload(ClassLoader classloader) {
              final SPIClassIterator<S> loader = SPIClassIterator.get(clazz, classloader);
              final LinkedHashMap<String,Class<? extends S>> services = new LinkedHashMap<String,Class<? extends S>>();
          

          ...shouldn't that LinkedHashMap be initialized with a copy of this.services (just like in NamedSPILoader.reload) so successive calls to reload(...) don't "forget" services that have already been added?

          (if you only call reload on child classloaders, then i imagine this wouldn't cause any problems, but with independent sibling classloaders it seems like calls stacks along the lines of..

          analysisloader = new AnalysisSPILoader(Foo.class, parentClassLoader);
          analysisloader.reload(childAClassLoader); 
          analysisloader.reload(childBClassLoader);
          

          ...would cause the loader to "forget" about any services it found in childAClassloader)

          Show
          Hoss Man added a comment - We have to fix the same issue in AnalysisSPILoader which is (unfortunately) a different class with some code duplication Ahhhh... that could totally explain why my naive attempt at fixing SOLR-4373 a while back didn't seem to work – i was only aware of NamedSPILoader but did ad hock testing using analyzer factories. Uwe: looking at your patch, one thing that jumps out at me is that AnalysisSPILoader seems to have another exist bug that may also cause some similar problems, regardless of thread safety... public synchronized void reload( ClassLoader classloader) { final SPIClassIterator<S> loader = SPIClassIterator.get(clazz, classloader); final LinkedHashMap< String , Class <? extends S>> services = new LinkedHashMap< String , Class <? extends S>>(); ...shouldn't that LinkedHashMap be initialized with a copy of this.services (just like in NamedSPILoader.reload) so successive calls to reload(...) don't "forget" services that have already been added? (if you only call reload on child classloaders, then i imagine this wouldn't cause any problems, but with independent sibling classloaders it seems like calls stacks along the lines of.. analysisloader = new AnalysisSPILoader(Foo.class, parentClassLoader); analysisloader.reload(childAClassLoader); analysisloader.reload(childBClassLoader); ...would cause the loader to "forget" about any services it found in childAClassloader)
          Hide
          Uwe Schindler added a comment -

          Thanks Hoss,
          this is indeed another bug. Too stupid! - copypaste error from the earlier days. In my opinion, thecode duplication is horrible, but AnalysisFactories unfortunately dont inplement NamedSPI, so have no name.

          Show
          Uwe Schindler added a comment - Thanks Hoss, this is indeed another bug. Too stupid! - copypaste error from the earlier days. In my opinion, thecode duplication is horrible, but AnalysisFactories unfortunately dont inplement NamedSPI, so have no name.
          Hide
          Hoss Man added a comment -

          +1 ... looks good to me.

          Show
          Hoss Man added a comment - +1 ... looks good to me.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Uwe Schindler
          http://svn.apache.org/viewvc?view=revision&revision=1450275

          LUCENE-4796, SOLR-4373: Fix concurrency issue in NamedSPILoader and AnalysisSPILoader when doing concurrent core loads in multicore Solr configs

          Show
          Commit Tag Bot added a comment - [trunk commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1450275 LUCENE-4796 , SOLR-4373 : Fix concurrency issue in NamedSPILoader and AnalysisSPILoader when doing concurrent core loads in multicore Solr configs
          Hide
          Uwe Schindler added a comment -

          I committed the fix for the concurrency bug and the incorrect reload starting with empty map instead of old map

          Show
          Uwe Schindler added a comment - I committed the fix for the concurrency bug and the incorrect reload starting with empty map instead of old map
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Uwe Schindler
          http://svn.apache.org/viewvc?view=revision&revision=1450276

          Merged revision(s) 1450275 from lucene/dev/trunk:
          LUCENE-4796, SOLR-4373: Fix concurrency issue in NamedSPILoader and AnalysisSPILoader when doing concurrent core loads in multicore Solr configs

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1450276 Merged revision(s) 1450275 from lucene/dev/trunk: LUCENE-4796 , SOLR-4373 : Fix concurrency issue in NamedSPILoader and AnalysisSPILoader when doing concurrent core loads in multicore Solr configs
          Hide
          Uwe Schindler added a comment -

          Closed after release.

          Show
          Uwe Schindler added a comment - Closed after release.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development