Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java

[Jacques Le Roux]

Hi,

The last commit is a problem as mentioned today in OFBIZ-9385

I'll see tomorrow morning...

Jacques

Le 05/12/2022 à 12:50, Jacques Le Roux a écrit :

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