On Mon, Dec 13, 2010 at 11:50 PM, Shai Erera <ser...@gmail.com> wrote:
> I see that the test was changed yesterday, and perhaps it causes the
> problem? Previously, the test had that code:
>
> // r might have changed because this is not a
> // synchronized method. However we don't want
> // to make it synchronized to test
> // thread-safety of IndexReader.close().
> // That's why we add refreshed also to
> // readersToClose, because double closing is fine
> if (refreshed != r) {
> refreshed.close();
> }
> readersToClose.add(refreshed);
>
> And Mike changed it to:
>
> if (refreshed != r) {
> refreshed.close();
> }
Yeah this comment made no sense to me, and neither did the double
close. Ie on getting the refreshedReader we should simply close it
and not add it to readersToClose.
> --> Removing the comment + adding refreshed to readersToClose. Maybe adding
> refreshed is still necessary?
Adding to readersToClose shouldn't be needed if we close it right away.
> I wasn't able to reproduce it on my environment though, so I'm not sure.
> Anyway, besides that change, the rest of the changes seem harmless and
> unrelated to the break (this one might not be related either, but it stands
> out as a potential problem).
This test failed yesterday (before my changes)... but actually now I
think I know where the bug is!
I think it's in PerFieldCodecWrapper -- its fieldsProducer method
fails to close the sub-fieldsProducers that were opened on hitting an
exception (which, in reopen, we can hit when writer is in the process
of committing).
I think we just need to add a try/finally there to close them... I'll do that.
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
