I ran a simplified version of the test code, with no sort but with a limit, and could not produce warnings. So I'd not do anything to change TopN unless/until we have more warnings to work with.
Have we stabilised enough that I turn this into a JIRA/PR with a unit test? Chris On 3 April 2017 at 15:34, Chris Dollin <[email protected]> wrote: > Argh, yes, all occurences of "open()" are supposed > to be "close()". > > | Have you seen warnings in the TopN case? > > I haven't got an isolated TopN test case but it has a similar > pattern of keeping references to the source iterator but > not closing via them. > > I can construct an example and see what happens. > > Chris > > > > > > On 3 April 2017 at 11:15, Andy Seaborne <[email protected]> wrote: > >> >> >> On 31/03/17 16:40, Chris Dollin wrote: >> >>> Hi Andy >>> >>> Yes, the cancel thread doesn't call open(), I understand >>> that. >>> >> >> is that "close()" not "open()"? >> >> The warnings are intermittent because the iterator >>> in question (the "source" interator of QueryIterSort) >>> is only left open if the cancel request is noticed while >>> the sort databag is filling up. >>> >>> QueryIterSort never explicitly closes the source >>> iterator. But if the sort databag pulls all of the >>> source bindings, the source iterator self-closes. >>> So there's a window while the bag is filling when >>> a detected cancellation will leave the source >>> iterator open. >>> >>> A fix is to ensure that the source iterator >>> is closed when QueryIterSort closes: >>> >>> @Override public void closeIterator() { >>> >> >> and also: >> >> this.db.close(); >> >> c.f. requestCancel >> >> embeddedIterator.close(); >>> super.closeIterator(); >>> } >>> >>> "embeddedIterator" is the source iterator. >>> >> >> SortedDataBag: add close() to cancel: >> >> public void cancel() { >> comparator.cancel(); >> close(); >> } >> >> >> >>> This fix eliminates the presenting problem and >>> the tests continue to pass /except/ that >>> CallBackIterator's close() method needs to >>> be made a no-op rather than failing, since >>> that open() is now actually called. >>> >> >> close()? >> >> >>> [It looks like QueryIterTopN has the same problem] >>> >> >> It would do no harm to have the same close processing in QueryIterTopN >> though it does not use data bags because it is an unconditional single pass >> over the data. >> >> Have you seen warnings in the TopN case? >> >> Andy >> >> >> >>> Chris >>> >>> >>> >>> On 20 March 2017 at 15:48, Chris Dollin <[email protected]> >>> wrote: >>> >>> On 17 March 2017 at 17:08, Andy Seaborne <[email protected]> wrote: >>>> >>>> >>>>> SortedDataBag.cancel does not call close. >>>>> >>>>> >>>> You're quite right, am having another look at the problem. >>>> >>>> Chris >>>> >>>> >>> >>> >>> > > > -- > "What I don't understand is this ..." Trevor Chaplin, /The Beiderbeck > Affair/ > > Epimorphics Ltd, http://www.epimorphics.com > Registered address: Court Lodge, 105 High Street, Portishead, Bristol BS20 > 6PT > Epimorphics Ltd. is a limited company registered in England (number > 7016688) > -- "What I don't understand is this ..." Trevor Chaplin, /The Beiderbeck Affair/ Epimorphics Ltd, http://www.epimorphics.com Registered address: Court Lodge, 105 High Street, Portishead, Bristol BS20 6PT Epimorphics Ltd. is a limited company registered in England (number 7016688)
