Thanks Deepak, Done as suggested
Jacques Le 05/12/2022 à 06:18, Deepak Dixit a écrit :
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 < [email protected]> 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 <[email protected]> <[email protected]> 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);
