I am working on some performance related thing, while reviewing code I found the code snippet where eli closed explicitly, That's why I landed up on this commit.
Please feel free to revert this commit, We can also remove explicitly eli.close call from handleOutput(), else will get unnecessary console warning from EntityListIterator.close() method. ``` Debug.logWarning("This EntityListIterator for Entity [" + modelEntityName + "] has already been closed, not closing again.", module); ``` Please feel free to Thanks & Regards -- Deepak Dixit ofbiz.apache.org On Sun, Dec 4, 2022 at 10:43 PM Jacques Le Roux < jacques.le.r...@les7arts.com> wrote: > Hi Deepak, > > You are right, I missed to conclude what I said at the end of the commit > comment ("I have to check the whole thing"). > So did not see that the called handleOutput() closes the eli var, well > spotted. > > Now if we look at both r1797097 > <http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java?r1=1797097&r2=1797096&pathrev=1797097> > and r1798086 > <http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java?r1=1798086&r2=1798085&pathrev=1798086> > we can see they really have no effects. r1797097 uses try-with-ressource > r1798086 removes it. > The question could be "what happens if we manually close a resource inside > a try-with-ressource block" (as in r1797097) > According to > https://stackoverflow.com/questions/21254547/manual-closing-inside-try-with-resource > it should be OK. > > So I finally think the real problem with r1798086 is the confusing comment > in code. Because in both cases (try-with-ressource or not) eli will be > closed by handleOutput . > > // NOTE: the eli EntityListIterator is not closed here. > It SHOULD be closed later after the returned list > will be used (eg see EntityAnd.getChildren() in ModelTree.java) > > Moreover it's confusing because ModelTree is completely unrelated. I > should have "check[ed] the whole thing" (maybe I confused the 2 > EntityFinderUtil::handleOutput) :/ > > Nevertheless, it sounds better reverting. In case an eli unrelated issue > happens inside handleOutput() before it closes eli. It's mostly > improbable but you never know :) > > I just have a question how did you get there? I guess you did not cross an > issue. So you stumbled upon this weird comment, right? > > I can revert if you like. > Thanks > > Jacques > Le 01/12/2022 à 09:04, Deepak Dixit a écrit : > > Hi Jacques, > > I know it's quite an old commit :) > > Do you remember the case where eli closed after the returned list has been > used? > > As handleOutput method closes the eli inline. > > I am planning to revert this commit, Please share your thoughts. > > > Thanks & Regards > -- > Deepak Dixitofbiz.apache.org > > > On Thu, Jun 8, 2017 at 9:46 PM <jler...@apache.org> <jler...@apache.org> > wrote: > > > Author: jleroux > Date: Thu Jun 8 16:16:29 2017 > New Revision: 1798086 > > URL: http://svn.apache.org/viewvc?rev=1798086&view=rev > Log: > This fixes a bug introduced with r1797097 > > As noted in code the EntityListIterator was not closed for a "good" > reason: it > might be used later by the framework though not passed as a var. I think I > found > 1 case where it's closed after the returned list have been used (in > EntityAnd.getChildren() in ModelTree.java); but I have to > > Modified: > > ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java > > Modified: > ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java > URL:http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java?rev=1798086&r1=1798085&r2=1798086&view=diff > > ============================================================================== > --- > ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java > (original) > +++ > ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java > Thu Jun 8 16:16:29 2017 > @@ -207,11 +207,13 @@ public abstract class ListFinder extends > options.setMaxRows(size * (index + 1)); > } > boolean beganTransaction = false; > - try (EntityListIterator eli = delegator.find(entityName, > whereEntityCondition, havingEntityCondition, fieldsToSelect, orderByFields, > options)) { > + try { > if (useTransaction) { > beganTransaction = TransactionUtil.begin(); > } > + EntityListIterator eli = delegator.find(entityName, > whereEntityCondition, havingEntityCondition, fieldsToSelect, orderByFields, > options); > this.outputHandler.handleOutput(eli, context, > listAcsr); > + // NOTE: the eli EntityListIterator is not closed > here. It SHOULD be closed later after the returned list will be used (eg > see EntityAnd.getChildren() in ModelTree.java) > } catch (GenericEntityException e) { > String errMsg = "Failure in by " + label + " find > operation, rolling back transaction"; > Debug.logError(e, errMsg, module); > > > > >