Skip to content

Commit

Permalink
Implemented withCloseable/try-with-resources where needed in the code…
Browse files Browse the repository at this point in the history 
… to ensure proper closure of the entity list iterator resource
  • Loading branch information
@dixitdeepak
dixitdeepak committed Nov 17, 2023
1 parent d82db61 commit f61cbb3
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 61 deletions.
5 changes: 1 addition & 4 deletions framework/service/org/moqui/impl/EntitySyncServices.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,9 @@ along with this software (see the LICENSE.md file). If not, see
// ec.logger.warn("=========== get#EntitySyncData entityName=${entryMap.entityName} count=${currentCount} find=${find}")
if (currentCount > 0) {
EntityListIterator resultEli = find.iterator()
try {
find.iterator().withCloseable ({resultEli ->
int levels = entryMap.dependents ? 2 : 0
resultEli.writeXmlText((Writer) entityWriter, null, levels)
} finally {
resultEli.close()
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions framework/src/main/groovy/org/moqui/impl/context/NotificationMessageImpl.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,8 @@ import org.moqui.BaseArtifactException
import org.moqui.Moqui
import org.moqui.context.ExecutionContext
import org.moqui.context.NotificationMessage
import org.moqui.context.NotificationMessage.NotificationType
import org.moqui.entity.EntityFacade
import org.moqui.entity.EntityList
import org.moqui.entity.EntityListIterator
import org.moqui.entity.EntityValue
import org.slf4j.Logger
import org.slf4j.LoggerFactory
Expand Down Expand Up @@ -93,16 +91,17 @@ class NotificationMessageImpl implements NotificationMessage, Externalizable {

// notify by group, skipping users already notified
if (userGroupId) {
EntityListIterator eli = ef.find("moqui.security.UserGroupMember")
ef.find("moqui.security.UserGroupMember")
.conditionDate("fromDate", "thruDate", new Timestamp(System.currentTimeMillis()))
.condition("userGroupId", userGroupId).disableAuthz().iterator()
.condition("userGroupId", userGroupId).disableAuthz().iterator().withCloseable ({eli ->
EntityValue nextValue
while ((nextValue = (EntityValue) eli.next()) != null) {
String userId = (String) nextValue.userId
if (checkedUserIds.contains(userId)) continue
checkedUserIds.add(userId)
if (checkUserNotify(userId, ef)) notifyUserIds.add(userId)
}
})
}

// add all users subscribed to all messages on the topic
Expand Down
12 changes: 4 additions & 8 deletions framework/src/main/groovy/org/moqui/impl/entity/EntityDataDocument.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,7 @@ class EntityDataDocument {
// do the one big query
String lastDocId = null
int docCount = 0
EntityListIterator mainEli = mainFind.iterator()
try {
try (EntityListIterator mainEli = mainFind.iterator()) {
logger.info("Feed dataDocumentId ${dataDocumentId} query complete (cursor opened) in ${System.currentTimeMillis() - startTimeMillis}ms")
EntityValue ev
while ((ev = (EntityValue) mainEli.next()) != null) {
Expand Down Expand Up @@ -345,7 +344,6 @@ class EntityDataDocument {
efi.ecfi.serviceFacade.sync().name(feedReceiveServiceName).parameter("documentList", documentMapList).call()
}
} finally {
mainEli.close()
logger.info("Feed dataDocumentId ${dataDocumentId} feed complete and cursor closed in ${System.currentTimeMillis() - startTimeMillis}ms")
}

Expand All @@ -366,16 +364,14 @@ class EntityDataDocument {
ArrayList<Map> documentMapList = ddi.hasAllPrimaryPks ? null : new ArrayList<Map>()

// do the one big query
EntityListIterator mainEli = mainFind.iterator()
try {

mainFind.iterator().withCloseable ({mainEli->
EntityValue ev
while ((ev = (EntityValue) mainEli.next()) != null) {
// logger.warn("=========== DataDocument query result for ${dataDocumentId}: ${ev}")
mergeValueToDocMap(ev, ddi, documentMapMap, documentMapList, docTsString)
}
} finally {
mainEli.close()
}
})

// make the actual list and return it
if (ddi.hasAllPrimaryPks) {
Expand Down
18 changes: 5 additions & 13 deletions framework/src/main/groovy/org/moqui/impl/entity/EntityDataWriterImpl.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,9 @@ class EntityDataWriterImpl implements EntityDataWriter {
EntityDefinition ed = efi.getEntityDefinition(en)
boolean useMaster = masterName != null && masterName.length() > 0 && ed.getMasterDefinition(masterName) != null
EntityFind ef = makeEntityFind(en)
EntityListIterator eli = ef.iterator()

try {

try (EntityListIterator eli = ef.iterator()) {
if (!eli.hasNext()) continue

String filename = path + '/' + en + '.' + fileType.name().toLowerCase()
Expand Down Expand Up @@ -200,8 +200,6 @@ class EntityDataWriterImpl implements EntityDataWriter {
} finally {
pw.close()
}
} finally {
eli.close()
}
}
} catch (Throwable t) {
Expand Down Expand Up @@ -248,8 +246,7 @@ class EntityDataWriterImpl implements EntityDataWriter {
EntityDefinition ed = efi.getEntityDefinition(en)
boolean useMaster = masterName != null && masterName.length() > 0 && ed.getMasterDefinition(masterName) != null
EntityFind ef = makeEntityFind(en)
EntityListIterator eli = ef.iterator()
try {
try (EntityListIterator eli = ef.iterator()) {
if (!eli.hasNext()) continue

String filenameBase = tableColumnNames ? ed.getTableName() : en
Expand All @@ -274,8 +271,6 @@ class EntityDataWriterImpl implements EntityDataWriter {
} finally {
out.closeEntry()
}
} finally {
eli.close()
}
}
} finally {
Expand Down Expand Up @@ -306,15 +301,12 @@ class EntityDataWriterImpl implements EntityDataWriter {
EntityDefinition ed = efi.getEntityDefinition(en)
boolean useMaster = masterName != null && masterName.length() > 0 && ed.getMasterDefinition(masterName) != null
EntityFind ef = makeEntityFind(en)
EntityListIterator eli = ef.iterator()
try {
ef.iterator().withCloseable ({
EntityValue ev
while ((ev = eli.next()) != null) {
valuesWritten+= writeValue(ev, writer, useMaster)
}
} finally {
eli.close()
}
});
}

endFile(writer)
Expand Down
39 changes: 14 additions & 25 deletions framework/src/main/groovy/org/moqui/impl/entity/EntityFindBase.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1149,19 +1149,18 @@ abstract class EntityFindBase implements EntityFind {
}

// call the abstract method
EntityListIterator eli
try { eli = iteratorExtended(queryWhereCondition, havingCondition, orderByExpanded, fieldInfoArray, fieldOptionsArray) }
try (EntityListIterator eli = iteratorExtended(queryWhereCondition, havingCondition, orderByExpanded, fieldInfoArray, fieldOptionsArray)) {
MNode databaseNode = this.efi.getDatabaseNode(ed.getEntityGroupName())
if (limit != null && databaseNode != null && "cursor".equals(databaseNode.attribute("offset-style"))) {
el = (EntityListImpl) eli.getPartialList(offset != null ? offset : 0, limit, false)
} else {
el = (EntityListImpl) eli.getCompleteList(false);
}
}
catch (SQLException e) { throw new EntitySqlException(makeErrorMsg("Error finding list of", LIST_ERROR, queryWhereCondition, ed, ec), e) }
catch (ArtifactAuthorizationException e) { throw e }
catch (Exception e) { throw new EntityException(makeErrorMsg("Error finding list of", LIST_ERROR, queryWhereCondition, ed, ec), e) }

MNode databaseNode = this.efi.getDatabaseNode(ed.getEntityGroupName())
if (limit != null && databaseNode != null && "cursor".equals(databaseNode.attribute("offset-style"))) {
el = (EntityListImpl) eli.getPartialList(offset != null ? offset : 0, limit, true)
} else {
el = (EntityListImpl) eli.getCompleteList(true)
}

// register lock after because we can't before, don't know which records will be returned
if (forUpdate && !isViewEntity && efi.ecfi.transactionFacade.getUseLockTrack()) {
int elSize = el.size()
Expand Down Expand Up @@ -1446,9 +1445,7 @@ abstract class EntityFindBase implements EntityFind {

this.useCache(false)
long totalUpdated = 0
EntityListIterator eli = (EntityListIterator) null
try {
eli = iterator()
iterator().withCloseable ({eli ->
EntityValue value
while ((value = eli.next()) != null) {
value.putAll(fieldsToSet)
Expand All @@ -1458,9 +1455,7 @@ abstract class EntityFindBase implements EntityFind {
totalUpdated++
}
}
} finally {
if (eli != null) eli.close()
}
})
return totalUpdated
}

Expand Down Expand Up @@ -1495,33 +1490,27 @@ abstract class EntityFindBase implements EntityFind {
}
} else {
this.resultSetConcurrency(ResultSet.CONCUR_UPDATABLE)
EntityListIterator eli = (EntityListIterator) null
try {
eli = iterator()
iterator().withCloseable ({eli->

while (eli.next() != null) {
// no longer need to clear cache, eli.remove() does that
eli.remove()
totalDeleted++
}
} finally {
if (eli != null) eli.close()
}
})
}
return totalDeleted
}

@Override
void extract(SimpleEtl etl) {
EntityListIterator eli = iterator()
try {
try (EntityListIterator eli = iterator()) {
EntityValue ev
while ((ev = eli.next()) != null) {
etl.processEntry(ev)
}
} catch (StopException e) {
logger.warn("EntityFind extract stopped on: " + (e.getCause()?.toString() ?: e.toString()))
} finally {
eli.close()
}
}

Expand Down
1 change: 1 addition & 0 deletions framework/src/main/groovy/org/moqui/impl/entity/EntityListIteratorImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ public EntityValueBase currentEntityValueBase() {
} catch (SQLException e) {
throw new EntityException("Error getting all results", e);
} finally {
//TODO: Remove closeAfter with respect to try-with-resource implementation
if (closeAfter) close();
}
}
Expand Down
10 changes: 3 additions & 7 deletions framework/src/test/groovy/EntityNoSqlCrud.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,23 +121,19 @@ class EntityNoSqlCrud extends Specification {

def "ELI find TestNoSqlEntity"() {
when:
EntityListIterator eli
EntityList partialEl = null
EntityValue first = null
try {
eli = ec.entity.find("moqui.test.TestNoSqlEntity")
.orderBy("-testNumberInteger").iterator()
try (EntityListIterator eli = ec.entity.find("moqui.test.TestNoSqlEntity")
.orderBy("-testNumberInteger").iterator()) {


partialEl = eli.getPartialList(0, 100, false)

eli.beforeFirst()
first = eli.next()
} catch (Exception e) {
logger.error("partialEl error", e)
} finally {
if (eli != null) eli.close()
}

// logger.warn("partialEl.size() ${partialEl.size()} first value ${first}")

then:
Expand Down

0 comments on commit f61cbb3

Please sign in to comment.