Lucene - Core
  1. Lucene - Core
  2. LUCENE-4953

readerClosedListener is not invoked for ParallelCompositeReader's leaves

    Details

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

      Description

      There was a test failure last night:

      1 tests failed.
      REGRESSION:  org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest.testBasic
      
      Error Message:
      testBasic(org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest): Insane FieldCache usage(s) found expected:<0> but was:<2>
      
      Stack Trace:
      java.lang.AssertionError: testBasic(org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest): Insane FieldCache usage(s) found expected:<0> but was:<2>
              at __randomizedtesting.SeedInfo.seed([1F9C2A2AD23A8E02:B466373F0DE6082C]:0)
              at org.junit.Assert.fail(Assert.java:93)
              at org.junit.Assert.failNotEquals(Assert.java:647)
              at org.junit.Assert.assertEquals(Assert.java:128)
              at org.junit.Assert.assertEquals(Assert.java:472)
              at org.apache.lucene.util.LuceneTestCase.assertSaneFieldCaches(LuceneTestCase.java:592)
              at org.apache.lucene.util.TestRuleFieldCacheSanity$1.evaluate(TestRuleFieldCacheSanity.java:55)
              at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46)
              at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
              at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:49)
              at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
              at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
              at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
              at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
              at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:782)
              at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:442)
              at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:746)
              at com.carrotsearch.randomizedtesting.RandomizedRunner$3.evaluate(RandomizedRunner.java:648)
              at com.carrotsearch.randomizedtesting.RandomizedRunner$4.evaluate(RandomizedRunner.java:682)
              at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:693)
              at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46)
              at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42)
              at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
              at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
              at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
              at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
              at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43)
              at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
              at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
              at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55)
              at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
              at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
              at java.lang.Thread.run(Thread.java:722)
      
      
      
      
      Build Log:
      [...truncated 6904 lines...]
      [junit4:junit4] Suite: org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest
      [junit4:junit4]   2> *** BEGIN testBasic(org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest): Insane FieldCache usage(s) ***
      [junit4:junit4]   2> VALUEMISMATCH: Multiple distinct value objects for ParallelAtomicReader(_0(5.0):C3)+id
      [junit4:junit4]   2>    'ParallelAtomicReader(_0(5.0):C3)'=>'id',class org.apache.lucene.index.SortedDocValues,0.5=>org.apache.lucene.search.FieldCacheImpl$SortedDocValuesImpl#386041791 (size =~ 232 bytes)
      [junit4:junit4]   2>    'ParallelAtomicReader(_0(5.0):C3)'=>'id',int,org.apache.lucene.search.FieldCache.DEFAULT_INT_PARSER=>org.apache.lucene.search.FieldCacheImpl$IntsFromArray#140912913 (size =~ 48 bytes)
      [junit4:junit4]   2>    'ParallelAtomicReader(_0(5.0):C3)'=>'id',int,null=>org.apache.lucene.search.FieldCacheImpl$IntsFromArray#140912913 (size =~ 48 bytes)
      [junit4:junit4]   2>
      [junit4:junit4]   2> VALUEMISMATCH: Multiple distinct value objects for ParallelAtomicReader(_1(5.0):C5)+id
      [junit4:junit4]   2>    'ParallelAtomicReader(_1(5.0):C5)'=>'id',int,null=>org.apache.lucene.search.FieldCacheImpl$IntsFromArray#1105632232 (size =~ 56 bytes)
      [junit4:junit4]   2>    'ParallelAtomicReader(_1(5.0):C5)'=>'id',int,org.apache.lucene.search.FieldCache.DEFAULT_INT_PARSER=>org.apache.lucene.search.FieldCacheImpl$IntsFromArray#1105632232 (size =~ 56 bytes)
      [junit4:junit4]   2>    'ParallelAtomicReader(_1(5.0):C5)'=>'id',class org.apache.lucene.index.SortedDocValues,0.5=>org.apache.lucene.search.FieldCacheImpl$SortedDocValuesImpl#27148040 (size =~ 232 bytes)
      [junit4:junit4]   2>
      [junit4:junit4]   2> *** END testBasic(org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest): Insane FieldCache usage(s) ***
      [junit4:junit4]   2> NOTE: download the large Jenkins line-docs file by running 'ant get-jenkins-line-docs' in the lucene directory.
      [junit4:junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=AllGroupHeadsCollectorTest -Dtests.method=testBasic -Dtests.seed=1F9C2A2AD23A8E02 -Dtests.multiplier=2 -Dtests.nightly=true -Dtests.slow=true -Dtests.linedocsfile=/home/hudson/lucene-data/enwiki.random.lines.txt -Dtests.locale=be_BY -Dtests.timezone=Asia/Manila -Dtests.file.encoding=US-ASCII
      [junit4:junit4] FAILURE 0.75s J1 | AllGroupHeadsCollectorTest.testBasic <<<
      [junit4:junit4]    > Throwable #1: java.lang.AssertionError: testBasic(org.apache.lucene.search.grouping.AllGroupHeadsCollectorTest): Insane FieldCache usage(s) found expected:<0> but was:<2>
      [junit4:junit4]    >    at __randomizedtesting.SeedInfo.seed([1F9C2A2AD23A8E02:B466373F0DE6082C]:0)
      [junit4:junit4]    >    at org.apache.lucene.util.LuceneTestCase.assertSaneFieldCaches(LuceneTestCase.java:592)
      [junit4:junit4]    >    at java.lang.Thread.run(Thread.java:722)
      [junit4:junit4]   2> NOTE: test params are: codec=Lucene42: {sort3=MockFixedIntBlock(blockSize=733), id=Pulsing41(freqCutoff=3 minBlockSize=50 maxBlockSize=177), content=MockFixedIntBlock(blockSize=733), author=Pulsing41(freqCutoff=3 minBlockSize=50 maxBlockSize=177), sort2=MockVariableIntBlock(baseBlockSize=71), sort1=Pulsing41(freqCutoff=3 minBlockSize=50 maxBlockSize=177), group=Pulsing41(freqCutoff=3 minBlockSize=50 maxBlockSize=177)}, docValues:{author_dv=DocValuesFormat(name=Disk), group_dv=DocValuesFormat(name=Disk)}, sim=RandomSimilarityProvider(queryNorm=false,coord=yes): {content=IB LL-L1, author=DFR GBZ(0.3)}, locale=be_BY, timezone=Asia/Manila
      [junit4:junit4]   2> NOTE: FreeBSD 9.0-RELEASE amd64/Oracle Corporation 1.7.0_17 (64-bit)/cpus=16,threads=1,free=157973280,total=249626624
      [junit4:junit4]   2> NOTE: All tests run in this JVM: [GroupFacetCollectorTest, AllGroupsCollectorTest, AllGroupHeadsCollectorTest]
      

      It reproduces, and happens because ParallelCompositeReader isn't invoking the reader listeners on its .leaves() when everything is closed. I made a separate test case to show the issue ...

      1. LUCENE-4953.patch
        15 kB
        Uwe Schindler
      2. LUCENE-4953.patch
        13 kB
        Uwe Schindler
      3. LUCENE-4953.patch
        11 kB
        Uwe Schindler
      4. LUCENE-4953.patch
        10 kB
        Uwe Schindler
      5. LUCENE-4953.patch
        10 kB
        Uwe Schindler
      6. LUCENE-4953.patch
        3 kB
        Uwe Schindler
      7. LUCENE-4953.patch
        4 kB
        Michael McCandless
      8. LUCENE-4953.patch
        2 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Test case showing the root cause of the failure ...

        I think we need to fix ParallelCompositeReader so that it actually closes the subReaders it creates?

        Show
        Michael McCandless added a comment - Test case showing the root cause of the failure ... I think we need to fix ParallelCompositeReader so that it actually closes the subReaders it creates?
        Hide
        Michael McCandless added a comment -

        Patch that "fixes" the test but ...

        It causes other test failures, because tests do this:

          searcher = newSearcher(reader);
          ...
          reader.close();
        

        (instead of searcher.getIndexReader().close()).

        Ie, today LuceneTestCase.maybeWrapReader never incRefs the incoming reader, but with the fix, ParallelCompositeReader now does ... so this leads to failures.

        Not sure what to do ...

        Maybe instead of this patch, we should just directly invoke the readerClosedListeners in ParallelCompositReader.doClose?

        Show
        Michael McCandless added a comment - Patch that "fixes" the test but ... It causes other test failures, because tests do this: searcher = newSearcher(reader); ... reader.close(); (instead of searcher.getIndexReader().close()). Ie, today LuceneTestCase.maybeWrapReader never incRefs the incoming reader, but with the fix, ParallelCompositeReader now does ... so this leads to failures. Not sure what to do ... Maybe instead of this patch, we should just directly invoke the readerClosedListeners in ParallelCompositReader.doClose?
        Hide
        Uwe Schindler added a comment - - edited

        As discussed in IRC today, like MultiReader/SlowComposite that warps another reader, we should no call close() on the subreaders. The problem here is: E.g., A wrapped slow reader still returns its delegate as core cache key, so the reader closed listener works as expected, when the child is closed. But PMR must not return the inner cache keys, as it wraps them with modifying index contents.

        The problem here is that the leaves are atomic and the field cache does not get the reader closed event. The somehow best fix would be to call readerClosedListener for all childs (not leaves!) on doClose. Theoretically, all CompositeReaders that wrap ll childs of other readers should do this (only Parallel is currently doing this).

        Another idea would be to use incRef and decRef, but that would not affect the wrapped atomic readers, so I prefer the previous solution.

        Show
        Uwe Schindler added a comment - - edited As discussed in IRC today, like MultiReader/SlowComposite that warps another reader, we should no call close() on the subreaders. The problem here is: E.g., A wrapped slow reader still returns its delegate as core cache key, so the reader closed listener works as expected, when the child is closed. But PMR must not return the inner cache keys, as it wraps them with modifying index contents. The problem here is that the leaves are atomic and the field cache does not get the reader closed event. The somehow best fix would be to call readerClosedListener for all childs (not leaves!) on doClose. Theoretically, all CompositeReaders that wrap ll childs of other readers should do this (only Parallel is currently doing this). Another idea would be to use incRef and decRef, but that would not affect the wrapped atomic readers, so I prefer the previous solution.
        Hide
        Uwe Schindler added a comment - - edited

        Ie, today LuceneTestCase.maybeWrapReader never incRefs the incoming reader, but with the fix, ParallelCompositeReader now does ... so this leads to failures.

        Passing false instead of true to the wrapper is wrong. The solution is to leave the construction as is, just in doClose() call notifyReaderClosedListeners() on all subreaders (which is unfortunately private). But this would still not help for tests, because maybeWrapReader tries to hide itsself from FieldCache (this is why it has FCInvisibleMultiReader). Tests almost always close the original reader and never the wrapper, so with this patch the whole thing does not work.

        The test failure is very seldom because it only happens:

        • if you wrap (rarely) with a ParallelMultiReader (more rarely)
        • use FieldCache

        The number of tests with FieldCache is very small, so it took more than 1 year to see the first failure

        In fact the problem for the actual test failure is maybeWrapReader in combination with FieldCache. maybeWrapReader should not use ParallelCompositeReader, if it knows that FieldCache will be used. Unfortunately this is not known before.

        The problem with the readerClosedListener not called by ParallelCompositeReader (with closeSubReaders=true) is real, and only affects PCR, because it creates atomic readers with "own" fieldcache key. Because of that it should manually unregister them (and not close them)

        Show
        Uwe Schindler added a comment - - edited Ie, today LuceneTestCase.maybeWrapReader never incRefs the incoming reader, but with the fix, ParallelCompositeReader now does ... so this leads to failures. Passing false instead of true to the wrapper is wrong. The solution is to leave the construction as is, just in doClose() call notifyReaderClosedListeners() on all subreaders (which is unfortunately private). But this would still not help for tests, because maybeWrapReader tries to hide itsself from FieldCache (this is why it has FCInvisibleMultiReader). Tests almost always close the original reader and never the wrapper, so with this patch the whole thing does not work. The test failure is very seldom because it only happens: if you wrap (rarely) with a ParallelMultiReader (more rarely) use FieldCache The number of tests with FieldCache is very small, so it took more than 1 year to see the first failure In fact the problem for the actual test failure is maybeWrapReader in combination with FieldCache. maybeWrapReader should not use ParallelCompositeReader, if it knows that FieldCache will be used. Unfortunately this is not known before. The problem with the readerClosedListener not called by ParallelCompositeReader (with closeSubReaders=true) is real, and only affects PCR, because it creates atomic readers with "own" fieldcache key. Because of that it should manually unregister them (and not close them)
        Hide
        Michael McCandless added a comment -

        It's ... spooky how PCR makes new (private) readers but never closes them But it seems hard to fix that correctly because of how LTC wraps the readers in newSearcher.

        +1 to just invoke the readerClosedListeners directly from PCR.doClose. It's a little un-clean duplicating this code but I don't see how else to fix it ...

        The number of tests with FieldCache is very small, so it took more than 1 year to see the first failure

        It's even more restrictive: the test must also create FieldCache insanity. This particular test always does so ... but because we purge the insanity on close (except in this case) the insanity never causes a test failure.

        Show
        Michael McCandless added a comment - It's ... spooky how PCR makes new (private) readers but never closes them But it seems hard to fix that correctly because of how LTC wraps the readers in newSearcher. +1 to just invoke the readerClosedListeners directly from PCR.doClose. It's a little un-clean duplicating this code but I don't see how else to fix it ... The number of tests with FieldCache is very small, so it took more than 1 year to see the first failure It's even more restrictive: the test must also create FieldCache insanity. This particular test always does so ... but because we purge the insanity on close (except in this case) the insanity never causes a test failure.
        Hide
        Uwe Schindler added a comment -

        Patch that notifies the readerClosedListeners of all childs instead of closing them completely. This is now correct, but very crazy.

        For this patch I made the notify method package protected, not sure if we should make it protected for other/similar readers?

        Show
        Uwe Schindler added a comment - Patch that notifies the readerClosedListeners of all childs instead of closing them completely. This is now correct, but very crazy. For this patch I made the notify method package protected, not sure if we should make it protected for other/similar readers?
        Hide
        Uwe Schindler added a comment -

        Of course the patch does not fix the test failures occuring because test never close the wrapper, but the original reader. This is a bug in maybeWrapReader (because maybeWrapReader should be side-effect free, but the side-effect here is that an entry in the FieldCache may be created by a private atomicreader instance which is just a wrapper and never closed).

        Show
        Uwe Schindler added a comment - Of course the patch does not fix the test failures occuring because test never close the wrapper, but the original reader. This is a bug in maybeWrapReader (because maybeWrapReader should be side-effect free, but the side-effect here is that an entry in the FieldCache may be created by a private atomicreader instance which is just a wrapper and never closed).
        Hide
        Uwe Schindler added a comment -

        Another possibility to make the readerClosed notification working, would be to have on PCR and PAR a 3rd mode (pkg-private ctor) where doClose() is a noop. In that case, the PCR could call close() on all of its subReaders, but the refCount or the childs are not closed unless otherwise specified in the ctor. And we dont need to make the private notify method visible to other classes.

        I will provide a patch tomorrow, this seems to be the cleanest solution for the readerClosedListener bug (but not the test failures).

        Show
        Uwe Schindler added a comment - Another possibility to make the readerClosed notification working, would be to have on PCR and PAR a 3rd mode (pkg-private ctor) where doClose() is a noop. In that case, the PCR could call close() on all of its subReaders, but the refCount or the childs are not closed unless otherwise specified in the ctor. And we dont need to make the private notify method visible to other classes. I will provide a patch tomorrow, this seems to be the cleanest solution for the readerClosedListener bug (but not the test failures).
        Hide
        Uwe Schindler added a comment -

        Patch that adds the DONT_TOUCH_SUBREADERS mode.

        I will now check the tests by enforcing the always wrapping with PCR, so bugs can be detected.

        Show
        Uwe Schindler added a comment - Patch that adds the DONT_TOUCH_SUBREADERS mode. I will now check the tests by enforcing the always wrapping with PCR, so bugs can be detected.
        Hide
        Uwe Schindler added a comment -

        I checked the other tests by hardcoding maybeWrapReader to always wrap with ParallelCompositeReader at the end. No other failures.

        I will commit this tomorrow.

        Show
        Uwe Schindler added a comment - I checked the other tests by hardcoding maybeWrapReader to always wrap with ParallelCompositeReader at the end. No other failures. I will commit this tomorrow.
        Hide
        Michael McCandless added a comment -

        I feel like we are making the wrong tradeoff here: we are making our core code (ParallelAtomic/CompositeReader) more hairy because of a limitation/constraint from LuceneTestCase.newSearcher (that it's never allowed to incRef the reader).

        I think instead we should fix that test limitation, and then use the original patch (where PCR incRefs the provided readers).

        EG we can move the reader wrapping to places like RandomIndexWriter.newReader instead of newSearcher ...

        Show
        Michael McCandless added a comment - I feel like we are making the wrong tradeoff here: we are making our core code (ParallelAtomic/CompositeReader) more hairy because of a limitation/constraint from LuceneTestCase.newSearcher (that it's never allowed to incRef the reader). I think instead we should fix that test limitation, and then use the original patch (where PCR incRefs the provided readers). EG we can move the reader wrapping to places like RandomIndexWriter.newReader instead of newSearcher ...
        Hide
        Uwe Schindler added a comment -

        I feel like we are making the wrong tradeoff here: we are making our core code (ParallelAtomic/CompositeReader) more hairy because of a limitation/constraint from LuceneTestCase.newSearcher (that it's never allowed to incRef the reader). I think instead we should fix that test limitation, and then use the original patch (where PCR incRefs the provided readers).

        These are 2 different limitations and 2 different bugs!

        If a user closes the ParallelCompositeReader it must free the field cache, so the fix here fixes a real bug - this bug was found by the test and your new test clearly shows it. Unfortunately my patch is a bit larger because some search/replace in it but actually does not change anything in the logic - it just internally changes a boolean to a 3-state enum. It's mostly just an automatic refactoring! In fact I was able to remove the crazy comment, because the code is now more clean (the hack with this comment explaing why closeSubReaders was true for the synthetic subreaders is now clarified by the code, which is much better!). The code is actually easier now, its maybe just the patch size that made you think its more complicated. I like the patch more than the hack+comment done before.

        The second bug which is indirectly related here is already solved (the test limitation): The 2nd bug is more about the fact that maybeWrapReader is not side-effect-free if used together with FieldCache+PCR -> thats the test bug and is not fixed by this issue (its just worked around by the grouping code that closes the wrapped reader, not the original reader). Ideally, tests working with fieldcache should call LTC.newSearcher(false), to prevent wrapping and the side effects by wrapping. Alternatively they have to close the wrapper not the original reader (so 2 possibilities to fix this bug). Gruping used the second one, which lead to the problems, because closing PCR did not correctly unlink its synthetic readers from fieldcache. But that was not a test issue.

        Show
        Uwe Schindler added a comment - I feel like we are making the wrong tradeoff here: we are making our core code (ParallelAtomic/CompositeReader) more hairy because of a limitation/constraint from LuceneTestCase.newSearcher (that it's never allowed to incRef the reader). I think instead we should fix that test limitation, and then use the original patch (where PCR incRefs the provided readers). These are 2 different limitations and 2 different bugs! If a user closes the ParallelCompositeReader it must free the field cache, so the fix here fixes a real bug - this bug was found by the test and your new test clearly shows it. Unfortunately my patch is a bit larger because some search/replace in it but actually does not change anything in the logic - it just internally changes a boolean to a 3-state enum. It's mostly just an automatic refactoring! In fact I was able to remove the crazy comment, because the code is now more clean (the hack with this comment explaing why closeSubReaders was true for the synthetic subreaders is now clarified by the code, which is much better!). The code is actually easier now, its maybe just the patch size that made you think its more complicated. I like the patch more than the hack+comment done before. The second bug which is indirectly related here is already solved (the test limitation): The 2nd bug is more about the fact that maybeWrapReader is not side-effect-free if used together with FieldCache+PCR -> thats the test bug and is not fixed by this issue (its just worked around by the grouping code that closes the wrapped reader, not the original reader). Ideally, tests working with fieldcache should call LTC.newSearcher(false), to prevent wrapping and the side effects by wrapping. Alternatively they have to close the wrapper not the original reader (so 2 possibilities to fix this bug). Gruping used the second one, which lead to the problems, because closing PCR did not correctly unlink its synthetic readers from fieldcache. But that was not a test issue.
        Hide
        Uwe Schindler added a comment -

        Here an updated patch. The naming inside the enum was made with help of beer. Now its named better and more correct.

        Users will not see any change, its completely private. No new enums/whatever.

        I think the code is now much easier to understand:

        • If user passes closeSubReaders=true, subreaders are closed
        • If user passes closeSubReaders=false, subreaders are increfed in ctor and decrefed in close (just like MultiReader does, too)

        The synthetic atomic/composite readers created internally differ completely from the public API. The "old" code was a hack:

        • The synthetic subreaders may not incref/decref, because the inref/decref is already done on the real readers
        • The synthetic subreaders may also not close the delegates/wrapped readers. Closing is also done by the parent. The synthetic readers are just a third kind and should behave as if they are not there.
        • The bug was, that we prevented in the hack by passing closeSubReaders=true, that they are increfed. As the old code never closed the synthetic readers, that was the "trick". The bug here was that because we never called close when the parent was closed, the FieldCache hanging on the synthetic atomic readers was not notified.
        • This patch just adds a third (private-only) mode, used solely in ParallelCompositeReader. When it creates the synthetic readers, those are not increfed/decrefed/closed (because doClose is noop). But We can still close them to inform the fieldcache. This is the only correct implementation. I am very sorry for the hack I did before 4.0
        Show
        Uwe Schindler added a comment - Here an updated patch. The naming inside the enum was made with help of beer. Now its named better and more correct. Users will not see any change, its completely private. No new enums/whatever. I think the code is now much easier to understand: If user passes closeSubReaders=true, subreaders are closed If user passes closeSubReaders=false, subreaders are increfed in ctor and decrefed in close (just like MultiReader does, too) The synthetic atomic/composite readers created internally differ completely from the public API. The "old" code was a hack: The synthetic subreaders may not incref/decref, because the inref/decref is already done on the real readers The synthetic subreaders may also not close the delegates/wrapped readers. Closing is also done by the parent. The synthetic readers are just a third kind and should behave as if they are not there. The bug was, that we prevented in the hack by passing closeSubReaders=true, that they are increfed. As the old code never closed the synthetic readers, that was the "trick". The bug here was that because we never called close when the parent was closed, the FieldCache hanging on the synthetic atomic readers was not notified. This patch just adds a third (private-only) mode, used solely in ParallelCompositeReader. When it creates the synthetic readers, those are not increfed/decrefed/closed (because doClose is noop). But We can still close them to inform the fieldcache. This is the only correct implementation. I am very sorry for the hack I did before 4.0
        Hide
        Michael McCandless added a comment -

        To fix the first bug, I would prefer that PCR makes "normal" private
        readers (that incRef their subs) and then closes those private readers
        in doClose, like my 2nd patch. I think that patch is much simpler
        than making a 3-way enum to tell PC/AR what to do on close.

        I realize this means wrapReader is no longer side-effect free, but I
        think that's a non-goal? That's a test-framework limitation, and we
        shouldn't let that mess up our core-code.

        Instead, I think we should fix the tests/test-framework: 1) fix
        newSearcher to no longer call wrapReader, 2) fix all tests that call
        newSearcher to call wrapReader instead and to close the wrapped
        reader. RandomIndexWriter.getReader can call wrapReader itself.

        Show
        Michael McCandless added a comment - To fix the first bug, I would prefer that PCR makes "normal" private readers (that incRef their subs) and then closes those private readers in doClose, like my 2nd patch. I think that patch is much simpler than making a 3-way enum to tell PC/AR what to do on close. I realize this means wrapReader is no longer side-effect free, but I think that's a non-goal? That's a test-framework limitation, and we shouldn't let that mess up our core-code. Instead, I think we should fix the tests/test-framework: 1) fix newSearcher to no longer call wrapReader, 2) fix all tests that call newSearcher to call wrapReader instead and to close the wrapped reader. RandomIndexWriter.getReader can call wrapReader itself.
        Hide
        Uwe Schindler added a comment -

        Hi,
        Mike and I had a lengthly discusssion on IRC. In short:

        The current semantics of MuliReader and other wrapper readers is the followng:

        • if you pass closeSubReaders=true (which is the default setting), the subreaders are not increfed, but close() is called if you close the wrapper. But you can also close the original reader -> it does not matter. This is the pattern well known from FilterInputStreams
        • If you pass closeSubReaders=false, the whole thing uses refCounting

        The problem with the patch and the refcounting is:

        • If we create a ParallelComposite with closeSubReader=true, we cannot change refcounts. But we do this when we create the synthetic subreaders with Mike's patch. This make the well-known FilterInputStream "pattern" no lomger working -> you have to close PCR

        The patch attached here is functional identical to my last one, it passes all tests (I also added another one with closesubreaders), but much simplier:

        The noop on doClose() is done by anonymous subclassing. I also made the top-level close simplier, because i just added the sequential synthetic subreaders to the IdentityHashSet which is iterated on doClose().

        I also added some tests for refcounting and forcefully closing the inner reader and checking that this correctly throws exception.

        Show
        Uwe Schindler added a comment - Hi, Mike and I had a lengthly discusssion on IRC. In short: The current semantics of MuliReader and other wrapper readers is the followng: if you pass closeSubReaders=true (which is the default setting), the subreaders are not increfed, but close() is called if you close the wrapper. But you can also close the original reader -> it does not matter. This is the pattern well known from FilterInputStreams If you pass closeSubReaders=false, the whole thing uses refCounting The problem with the patch and the refcounting is: If we create a ParallelComposite with closeSubReader=true, we cannot change refcounts. But we do this when we create the synthetic subreaders with Mike's patch. This make the well-known FilterInputStream "pattern" no lomger working -> you have to close PCR The patch attached here is functional identical to my last one, it passes all tests (I also added another one with closesubreaders), but much simplier: The noop on doClose() is done by anonymous subclassing. I also made the top-level close simplier, because i just added the sequential synthetic subreaders to the IdentityHashSet which is iterated on doClose(). I also added some tests for refcounting and forcefully closing the inner reader and checking that this correctly throws exception.
        Hide
        Uwe Schindler added a comment -

        I forgot: the patch also removes toString() because this is handled by CompositeReader base class already.

        Show
        Uwe Schindler added a comment - I forgot: the patch also removes toString() because this is handled by CompositeReader base class already.
        Hide
        Uwe Schindler added a comment -

        Instead, I think we should fix the tests/test-framework: 1) fix
        newSearcher to no longer call wrapReader, 2) fix all tests that call
        newSearcher to call wrapReader instead and to close the wrapped
        reader. RandomIndexWriter.getReader can call wrapReader itself.

        +1, we should do this in a separate issue.

        Show
        Uwe Schindler added a comment - Instead, I think we should fix the tests/test-framework: 1) fix newSearcher to no longer call wrapReader, 2) fix all tests that call newSearcher to call wrapReader instead and to close the wrapped reader. RandomIndexWriter.getReader can call wrapReader itself. +1, we should do this in a separate issue.
        Hide
        Uwe Schindler added a comment -

        More funny refcount tests.

        Mike agreed on IRC so this should be ready to commit.

        Show
        Uwe Schindler added a comment - More funny refcount tests. Mike agreed on IRC so this should be ready to commit.
        Hide
        Michael McCandless added a comment -

        +1, thanks Uwe!

        Show
        Michael McCandless added a comment - +1, thanks Uwe!
        Hide
        Uwe Schindler added a comment -

        This patch contains in addition a test for toString() and a fix in CompositeReader to create a good toString() representation if this is an anonymous class (in that case Class#getSimpleName() returns ""). There are more places in Lucene that could be affected by this, just as warning! Maybe we add a helper method to get a simple non-empty class name.

        Show
        Uwe Schindler added a comment - This patch contains in addition a test for toString() and a fix in CompositeReader to create a good toString() representation if this is an anonymous class (in that case Class#getSimpleName() returns ""). There are more places in Lucene that could be affected by this, just as warning! Maybe we add a helper method to get a simple non-empty class name.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] uschindler
        http://svn.apache.org/viewvc?view=revision&revision=1476526

        LUCENE-4953: Fixed ParallelCompositeReader to inform ReaderClosedListeners of its synthetic subreaders. FieldCaches keyed on the atomic childs will be purged earlier and FC insanity prevented. In addition, ParallelCompositeReader's toString() was changed to better reflect the reader structure.

        Show
        Commit Tag Bot added a comment - [trunk commit] uschindler http://svn.apache.org/viewvc?view=revision&revision=1476526 LUCENE-4953 : Fixed ParallelCompositeReader to inform ReaderClosedListeners of its synthetic subreaders. FieldCaches keyed on the atomic childs will be purged earlier and FC insanity prevented. In addition, ParallelCompositeReader's toString() was changed to better reflect the reader structure.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] uschindler
        http://svn.apache.org/viewvc?view=revision&revision=1476529

        Merged revision(s) 1476526 from lucene/dev/trunk:
        LUCENE-4953: Fixed ParallelCompositeReader to inform ReaderClosedListeners of its synthetic subreaders. FieldCaches keyed on the atomic childs will be purged earlier and FC insanity prevented. In addition, ParallelCompositeReader's toString() was changed to better reflect the reader structure.

        Show
        Commit Tag Bot added a comment - [branch_4x commit] uschindler http://svn.apache.org/viewvc?view=revision&revision=1476529 Merged revision(s) 1476526 from lucene/dev/trunk: LUCENE-4953 : Fixed ParallelCompositeReader to inform ReaderClosedListeners of its synthetic subreaders. FieldCaches keyed on the atomic childs will be purged earlier and FC insanity prevented. In addition, ParallelCompositeReader's toString() was changed to better reflect the reader structure.
        Hide
        Uwe Schindler added a comment -

        Thanks Mike for the fruitful discussions!

        Show
        Uwe Schindler added a comment - Thanks Mike for the fruitful discussions!
        Hide
        Steve Rowe added a comment -

        If there are no objections, I'd like to backport this to 4.3.1.

        Show
        Steve Rowe added a comment - If there are no objections, I'd like to backport this to 4.3.1.
        Hide
        Shalin Shekhar Mangar added a comment -

        Back ported to 4.3.1 r1483272

        Show
        Shalin Shekhar Mangar added a comment - Back ported to 4.3.1 r1483272
        Hide
        Shalin Shekhar Mangar added a comment -

        Bulk closing after 4.3.1 release

        Show
        Shalin Shekhar Mangar added a comment - Bulk closing after 4.3.1 release

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development