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 Dixit
ofbiz.apache.org
On Thu, Jun 8, 2017 at 9:46 PM<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);
