On 4 April 2017 at 13:39, Chris Dollin <[email protected]> wrote:
>> On 4 April 2017 at 09:37, Chris Dollin <[email protected]> >>> wrote: >>> >>>> I ran a simplified version of the test code, with no >>>> sort but with a limit, and could not produce warnings. >>>> >>> >> Well of course not, it needs sort + small limit to involve TopN >> >> So. Testing with a version of Fuseki that has the fix for QueryIterSort included, with the query having LIMIT 2 to have TopN used, we get the same (infrequent) warning messages. Making the same change to TopN as we made to Sort and the problem goes away. >> >> >>> 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) >>>> >>> >>> >>> >>> -- >>> "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) >> > > > > -- > "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)
