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);
>
>
>
>
>
