OK I just committed a fix. Hopefully this test stops failing now :) Mike
On Tue, Dec 14, 2010 at 5:53 AM, Michael McCandless <[email protected]> wrote: > On Mon, Dec 13, 2010 at 11:50 PM, Shai Erera <[email protected]> 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: [email protected] For additional commands, e-mail: [email protected]
