svn commit: r1798566 - in /ofbiz/ofbiz-framework/trunk: applications/party/src/main/java/org/apache/ofbiz/party/party/ applications/product/src/main/java/org/apache/ofbiz/product/feature/ applications/product/src/main/java/org/apache/ofbiz/product/prod...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

svn commit: r1798566 - in /ofbiz/ofbiz-framework/trunk: applications/party/src/main/java/org/apache/ofbiz/party/party/ applications/product/src/main/java/org/apache/ofbiz/product/feature/ applications/product/src/main/java/org/apache/ofbiz/product/prod...

jleroux@apache.org
Author: jleroux
Date: Tue Jun 13 08:29:28 2017
New Revision: 1798566

URL: http://svn.apache.org/viewvc?rev=1798566&view=rev
Log:
Improved: EntityListIterator closed but not in case of exception
(OFBIZ-9385)

This is an improvement only because no cases were reported. But obviously in
case of unlucky exception after the EntityListIterator creation and before it's
closed the EntityListIterator remains in memory.
It should be closed in EntityListIterator.finalize() but the less happens there
the better.

The solution is to use try-with-ressources when possible

This completes OFBIZ-9385, there are no other cases left.

There are few methods like FindServices.executeFind() or performFindParty
service which can't be improved because they return an EntityListIterator.
So methods using them must be carefully crafted to close the EntityListIterator.
Most where already documented (in the framework). I completed few in
applications at r1798495  



Modified:
    ofbiz/ofbiz-framework/trunk/applications/party/src/main/java/org/apache/ofbiz/party/party/PartyServices.java
    ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/feature/ParametricSearch.java
    ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductSearch.java
    ofbiz/ofbiz-framework/trunk/framework/entityext/src/main/java/org/apache/ofbiz/entityext/synchronization/EntitySyncContext.java
    ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java

Modified: ofbiz/ofbiz-framework/trunk/applications/party/src/main/java/org/apache/ofbiz/party/party/PartyServices.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/party/src/main/java/org/apache/ofbiz/party/party/PartyServices.java?rev=1798566&r1=1798565&r2=1798566&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/applications/party/src/main/java/org/apache/ofbiz/party/party/PartyServices.java (original)
+++ ofbiz/ofbiz-framework/trunk/applications/party/src/main/java/org/apache/ofbiz/party/party/PartyServices.java Tue Jun 13 08:29:28 2017
@@ -954,7 +954,7 @@ public class PartyServices {
         Locale locale = (Locale) context.get("locale");
 
         try {
-         parties = EntityQuery.use(delegator).from("Party")
+            parties = EntityQuery.use(delegator).from("Party")
                     .where(EntityCondition.makeCondition(EntityFunction.UPPER_FIELD("externalId"), EntityOperator.EQUALS, EntityFunction.UPPER(externalId)))
                     .orderBy("externalId", "partyId")
                     .queryList();
@@ -1502,21 +1502,19 @@ public class PartyServices {
             
             // do the lookup
             if (mainCond != null || "Y".equals(showAll)) {
-                try {
-                    // get the indexes for the partial list
-                    lowIndex = viewIndex * viewSize + 1;
-                    highIndex = (viewIndex + 1) * viewSize;
-
-                    // set distinct on so we only get one row per order
-                    // using list iterator
-                    EntityListIterator pli = EntityQuery.use(delegator).select(UtilMisc.toSet(fieldsToSelect))
-                            .from(dynamicView)
-                            .where(mainCond)
-                            .orderBy(orderBy)
-                            .cursorScrollInsensitive()
-                            .fetchSize(highIndex)
-                            .distinct()
-                            .queryIterator();
+                lowIndex = viewIndex * viewSize + 1;
+                highIndex = (viewIndex + 1) * viewSize;
+
+                // set distinct on so we only get one row per order
+                // using list iterator
+                EntityQuery eq = EntityQuery.use(delegator).select(UtilMisc.toSet(fieldsToSelect))
+                        .from(dynamicView)
+                        .where(mainCond)
+                        .orderBy(orderBy)
+                        .cursorScrollInsensitive()
+                        .fetchSize(highIndex)
+                        .distinct();
+                try (EntityListIterator pli = eq.queryIterator()) {
 
                     // get the partial list for this page
                     partyList = pli.getPartialList(lowIndex, viewSize);
@@ -1527,8 +1525,6 @@ public class PartyServices {
                         highIndex = partyListSize;
                     }
 
-                    // close the list iterator
-                    pli.close();
                 } catch (GenericEntityException e) {
                     String errMsg = "Failure in party find operation, rolling back transaction: " + e.toString();
                     Debug.logError(e, errMsg, module);

Modified: ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/feature/ParametricSearch.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/feature/ParametricSearch.java?rev=1798566&r1=1798565&r2=1798566&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/feature/ParametricSearch.java (original)
+++ ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/feature/ParametricSearch.java Tue Jun 13 08:29:28 2017
@@ -123,9 +123,8 @@ public class ParametricSearch {
     }
     public static Map<String, List<GenericValue>> getAllFeaturesByType(Delegator delegator, int perTypeMaxSize) {
         Map<String, List<GenericValue>> productFeaturesByTypeMap = new HashMap<String, List<GenericValue>>();
-        try {
-            Set<String> typesWithOverflowMessages = new HashSet<String>();
-            EntityListIterator productFeatureEli = EntityQuery.use(delegator).from("ProductFeature").orderBy("description").queryIterator();
+        Set<String> typesWithOverflowMessages = new HashSet<String>();
+        try (EntityListIterator productFeatureEli = EntityQuery.use(delegator).from("ProductFeature").orderBy("description").queryIterator()) {
             GenericValue productFeature = null;
             while ((productFeature = productFeatureEli.next()) != null) {
                 String productFeatureTypeId = productFeature.getString("productFeatureTypeId");
@@ -143,7 +142,6 @@ public class ParametricSearch {
                     featuresByType.add(productFeature);
                 }
             }
-            productFeatureEli.close();
         } catch (GenericEntityException e) {
             Debug.logError(e, "Error getting all features", module);
         }

Modified: ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductSearch.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductSearch.java?rev=1798566&r1=1798565&r2=1798566&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductSearch.java (original)
+++ ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductSearch.java Tue Jun 13 08:29:28 2017
@@ -222,14 +222,12 @@ public class ProductSearch {
             long startMillis = System.currentTimeMillis();
 
             // do the query
-            EntityListIterator eli = this.doQuery(delegator);
-            ArrayList<String> productIds = this.makeProductIdList(eli);
-            if (eli != null) {
-                try {
-                    eli.close();
-                } catch (GenericEntityException e) {
-                    Debug.logError(e, "Error closing ProductSearch EntityListIterator");
-                }
+            ArrayList<String> productIds = null;
+            try (EntityListIterator eli = this.doQuery(delegator)) {
+                productIds = this.makeProductIdList(eli);
+            } catch (GenericEntityException e) {
+                Debug.logError(e, module);
+                return null;
             }
 
             long endMillis = System.currentTimeMillis();
@@ -649,6 +647,12 @@ public class ProductSearch {
             if (Debug.infoOn()) Debug.logInfo("topCond=" + topCond.makeWhereString(null, new LinkedList<EntityConditionParam>(), EntityConfig.getDatasource(delegator.getEntityHelperName("Product"))), module);
         }
 
+        /**
+         * @param delegator the delegator
+         * @return EntityListIterator representing the result of the query: NOTE THAT THIS MUST BE CLOSED WHEN YOU ARE
+         *      DONE WITH IT (preferably in a finally block),
+         *      AND DON'T LEAVE IT OPEN TOO LONG BECAUSE IT WILL MAINTAIN A DATABASE CONNECTION.
+         */
         public EntityListIterator doQuery(Delegator delegator) {
             // handle the now assembled or and and keyword fixed lists
             this.finishKeywordConstraints();

Modified: ofbiz/ofbiz-framework/trunk/framework/entityext/src/main/java/org/apache/ofbiz/entityext/synchronization/EntitySyncContext.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/entityext/src/main/java/org/apache/ofbiz/entityext/synchronization/EntitySyncContext.java?rev=1798566&r1=1798565&r2=1798566&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/entityext/src/main/java/org/apache/ofbiz/entityext/synchronization/EntitySyncContext.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/entityext/src/main/java/org/apache/ofbiz/entityext/synchronization/EntitySyncContext.java Tue Jun 13 08:29:28 2017
@@ -345,35 +345,39 @@ public class EntitySyncContext {
             }
 
             try {
-                // get the values created within the current time range
                 EntityCondition findValCondition = EntityCondition.makeCondition(
                         EntityCondition.makeCondition(ModelEntity.CREATE_STAMP_TX_FIELD, EntityOperator.GREATER_THAN_EQUAL_TO, currentRunStartTime),
                         EntityCondition.makeCondition(ModelEntity.CREATE_STAMP_TX_FIELD, EntityOperator.LESS_THAN, currentRunEndTime));
-                EntityListIterator eli = EntityQuery.use(delegator)
-                                                    .from(modelEntity.getEntityName())
-                                                    .where(findValCondition)
-                                                    .orderBy(ModelEntity.CREATE_STAMP_TX_FIELD, ModelEntity.CREATE_STAMP_FIELD)
-                                                    .queryIterator();
-                GenericValue nextValue = null;
+                EntityQuery eq = EntityQuery.use(delegator)
+                        .from(modelEntity.getEntityName())
+                        .where(findValCondition)
+                        .orderBy(ModelEntity.CREATE_STAMP_TX_FIELD, ModelEntity.CREATE_STAMP_FIELD);
+
                 long valuesPerEntity = 0;
-                while ((nextValue = eli.next()) != null) {
-                    // sort by the tx stamp and then the record stamp
-                    // find first value in valuesToCreate list, starting with the current insertBefore value, that has a CREATE_STAMP_TX_FIELD after the nextValue.CREATE_STAMP_TX_FIELD, then do the same with CREATE_STAMP_FIELD
-                    while (insertBefore < valuesToCreate.size() && valuesToCreate.get(insertBefore).getTimestamp(ModelEntity.CREATE_STAMP_TX_FIELD).before(nextValue.getTimestamp(ModelEntity.CREATE_STAMP_TX_FIELD))) {
-                        insertBefore++;
+                try (EntityListIterator eli = eq.queryIterator()) {
+                    // get the values created within the current time range
+                    GenericValue nextValue = null;
+
+                    while ((nextValue = eli.next()) != null) {
+                        // sort by the tx stamp and then the record stamp
+                        // find first value in valuesToCreate list, starting with the current insertBefore value, that has a CREATE_STAMP_TX_FIELD after the nextValue.CREATE_STAMP_TX_FIELD, then do the same with CREATE_STAMP_FIELD
+                        while (insertBefore < valuesToCreate.size() && valuesToCreate.get(insertBefore).getTimestamp(ModelEntity.CREATE_STAMP_TX_FIELD).before(nextValue.getTimestamp(ModelEntity.CREATE_STAMP_TX_FIELD))) {
+                            insertBefore++;
+                        }
+                        while (insertBefore < valuesToCreate.size() && valuesToCreate.get(insertBefore).getTimestamp(ModelEntity.CREATE_STAMP_FIELD).before(nextValue.getTimestamp(ModelEntity.CREATE_STAMP_FIELD))) {
+                            insertBefore++;
+                        }
+                        valuesToCreate.add(insertBefore, nextValue);
+                        valuesPerEntity++;
                     }
-                    while (insertBefore < valuesToCreate.size() && valuesToCreate.get(insertBefore).getTimestamp(ModelEntity.CREATE_STAMP_FIELD).before(nextValue.getTimestamp(ModelEntity.CREATE_STAMP_FIELD))) {
-                        insertBefore++;
+                } catch (GenericEntityException e) {
+                    try {
+                        TransactionUtil.rollback(beganTransaction, "Entity Engine error in assembleValuesToCreate", e);
+                    } catch (GenericTransactionException e2) {
+                        Debug.logWarning(e2, "Unable to call rollback()", module);
                     }
-                    valuesToCreate.add(insertBefore, nextValue);
-                    valuesPerEntity++;
+                    throw new SyncDataErrorException("Error getting values to create from the datasource", e);
                 }
-                eli.close();
-
-                // definately remove this message and related data gathering
-                //long preCount = delegator.findCountByCondition(modelEntity.getEntityName(), findValCondition, null);
-                //long entityTotalCount = delegator.findCountByCondition(modelEntity.getEntityName(), null, null);
-                //if (entityTotalCount > 0 || preCount > 0 || valuesPerEntity > 0) Debug.logInfo("Got " + valuesPerEntity + "/" + preCount + "/" + entityTotalCount + " values for entity " + modelEntity.getEntityName(), module);
 
                 // if we didn't find anything for this entity, find the next value's Timestamp and keep track of it
                 if (valuesPerEntity == 0) {
@@ -382,14 +386,24 @@ public class EntitySyncContext {
                     EntityCondition findNextCondition = EntityCondition.makeCondition(
                             EntityCondition.makeCondition(ModelEntity.CREATE_STAMP_TX_FIELD, EntityOperator.NOT_EQUAL, null),
                             EntityCondition.makeCondition(ModelEntity.CREATE_STAMP_TX_FIELD, EntityOperator.GREATER_THAN_EQUAL_TO, currentRunEndTime));
-                    EntityListIterator eliNext = EntityQuery.use(delegator)
-                                                            .from(modelEntity.getEntityName())
-                                                            .where(findNextCondition)
-                                                            .orderBy(ModelEntity.CREATE_STAMP_TX_FIELD)
-                                                            .queryIterator();
-                    // get the first element and it's tx time value...
-                    GenericValue firstVal = eliNext.next();
-                    eliNext.close();
+                    eq = EntityQuery.use(delegator)
+                            .from(modelEntity.getEntityName())
+                            .where(findNextCondition)
+                            .orderBy(ModelEntity.CREATE_STAMP_TX_FIELD);
+
+                    GenericValue firstVal = null;
+                    try (EntityListIterator eliNext = eq.queryIterator()) {
+                        // get the first element and it's tx time value...
+                        firstVal = eliNext.next();
+                    } catch (GenericEntityException e) {
+                        try {
+                            TransactionUtil.rollback(beganTransaction, "Entity Engine error in assembleValuesToCreate", e);
+                        } catch (GenericTransactionException e2) {
+                            Debug.logWarning(e2, "Unable to call rollback()", module);
+                        }
+                        throw new SyncDataErrorException("Error getting values to create from the datasource", e);
+                    }
+                    
                     Timestamp nextTxTime;
                     if (firstVal != null) {
                         nextTxTime = firstVal.getTimestamp(ModelEntity.CREATE_STAMP_TX_FIELD);
@@ -401,20 +415,13 @@ public class EntitySyncContext {
                         this.nextCreateTxTime = nextTxTime;
                         Debug.logInfo("EntitySync: Set nextCreateTxTime to [" + nextTxTime + "]", module);
                     }
+                    
                     Timestamp curEntityNextTxTime = this.nextEntityCreateTxTime.get(modelEntity.getEntityName());
                     if (curEntityNextTxTime == null || nextTxTime.before(curEntityNextTxTime)) {
                         this.nextEntityCreateTxTime.put(modelEntity.getEntityName(), nextTxTime);
                         Debug.logInfo("EntitySync: Set nextEntityCreateTxTime to [" + nextTxTime + "] for the entity [" + modelEntity.getEntityName() + "]", module);
                     }
                 }
-            } catch (GenericEntityException e) {
-                try {
-                    TransactionUtil.rollback(beganTransaction, "Entity Engine error in assembleValuesToCreate", e);
-
-                } catch (GenericTransactionException e2) {
-                    Debug.logWarning(e2, "Unable to call rollback()", module);
-                }
-                throw new SyncDataErrorException("Error getting values to create from the datasource", e);
             } catch (Throwable t) {
                 try {
                     TransactionUtil.rollback(beganTransaction, "Throwable error in assembleValuesToCreate", t);
@@ -423,7 +430,7 @@ public class EntitySyncContext {
                 }
                 throw new SyncDataErrorException("Caught runtime error while getting values to create", t);
             }
-
+            
             try {
                 TransactionUtil.commit(beganTransaction);
             } catch (GenericTransactionException e) {
@@ -449,13 +456,13 @@ public class EntitySyncContext {
             }
             Debug.logInfo(toCreateInfo.toString(), module);
         }
-        
+
         // As the this.nextCreateTxTime calculation is only based on entities without values to create, if there at least one value to create returned
         // this calculation is false, so it needs to be nullified
         if (valuesToCreate.size() > 0) {
             this.nextCreateTxTime = null;
         }
-        
+
         return valuesToCreate;
     }
 

Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java?rev=1798566&r1=1798565&r2=1798566&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java Tue Jun 13 08:29:28 2017
@@ -396,14 +396,12 @@ public class ModelTree extends ModelWidg
                 // List dataFound = (List)context.get("dataFound");
                 Iterator<? extends Map<String, ? extends Object>> dataIter = subNode.getListIterator(context);
                 if (dataIter instanceof EntityListIterator) {
-                    EntityListIterator eli = (EntityListIterator) dataIter;
-                    Map<String, Object> val = null;
-                    while ((val = eli.next()) != null) {
-                        Object[] arr = { node, val };
-                        subNodeValues.add(arr);
-                    }
-                    try {
-                        eli.close();
+                    try (EntityListIterator eli = (EntityListIterator) dataIter) {
+                        Map<String, Object> val = null;
+                        while ((val = eli.next()) != null) {
+                            Object[] arr = { node, val };
+                            subNodeValues.add(arr);
+                        }
                     } catch (GenericEntityException e) {
                         Debug.logError(e, module);
                         throw new RuntimeException(e.getMessage());