You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Deepak Dixit <de...@apache.org> on 2022/12/01 08:04:33 UTC
Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java
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 <jl...@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 check the whole
> thing
>
> 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);
>
>
>
Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Deepak,
I had to revert. I also added a better comment for future. Please see OFBIZ-9385 for details if necessary
Jacques
Le 06/12/2022 à 19:21, Jacques Le Roux a écrit :
> 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.roux@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 <jl...@apache.org> <jl...@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);
>>>>
>>>>
>>>>
>>>>
>>>>
Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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.roux@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 <jl...@apache.org> <jl...@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);
>>>
>>>
>>>
>>>
>>>
Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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.roux@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 <jl...@apache.org> <jl...@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);
>>
>>
>>
>>
>>
Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java
Posted by Deepak Dixit <de...@apache.org>.
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.roux@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 <jl...@apache.org> <jl...@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);
>
>
>
>
>
Re: svn commit: r1798086 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbiz/entity/finder/ListFinder.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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<jl...@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);
>>
>>
>>