Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented withCloseable/try-with-resources where needed in the code… #625

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

dixitdeepak
Copy link
Contributor

@dixitdeepak dixitdeepak commented Nov 17, 2023

Initial cleanup of Entity List Iterator for proper resource closure.

This PR addresses occurrences in the codebase where withCloseable/try-with-resources is applied to ensure proper closure of resources.
Additional attention is needed in xmlActions.groovy.ftl, especially where MiniLang is utilized.

@jonesde
Copy link
Member

jonesde commented Nov 21, 2023

For what it's worth, I personally don't like the try with resources code pattern for most things, and where an EntityListIterator is already closed in a finally block I don't see a reason for changing it (and would even prefer not to).

I scanned through and didn't see any, but were there any functional changes in there or places where an ELI was not properly closed?

@dixitdeepak
Copy link
Contributor Author

Thanks David for the review,
We've encountered memory leaks during the generation of feed files from the database using EntityListIterator (ELI) and subsequent SFTP operations. While we've successfully addressed resource management in the SFTP code by implementing ARM and have observed a reduction in memory leaks, we still face the challenge of server restarts every two weeks to prevent heap memory from reaching 90%.

@dixitdeepak
Copy link
Contributor Author

I am referring the following comment from ARM

However, this example might have a resource leak. A program has to do more than rely on the garbage collector (GC) to reclaim a resource's memory when it's finished with it. The program must also release the resoure back to the operating system, typically by calling the resource's close method. However, if a program fails to do this before the GC reclaims the resource, then the information needed to release the resource is lost. The resource, which is still considered by the operating system to be in use, has leaked.

And as per the The finally Block document
Important: Use a try-with-resources statement instead of a finally block when closing a file or otherwise recovering resources.

As per the above comments
If we want to release resources properly, will have to use try with resource for ARM.

@jonesde
Copy link
Member

jonesde commented Nov 29, 2023

I read through that try-with-resources statement doc, and what it is pointing out is that if you have TWO close() calls in a finally block and the first throws an exception then the second one won't be called. This doesn't mean that the finally approach is invalid, but in cases like that (which you'll see in certain places in moqui, like in the ELI close() method itself with the ResultSet and Connection both needing to be closed) that each close() has to be wrapped in a try/catch so that an exception in the first won't cause the second to be missed.

If it was possible to declare the field inside the try block and still have it referenced in the finally block for closing that could also result in a GC before close is called, but since that is not possible (will result in a compile error) you don't have to worry about that.

That said, from reviewing the changes again it looks like you found two places where close() was not being called on the eli, so thank you for catching those and I'll go ahead and merge all of this.

@jonesde jonesde merged commit e6135fa into moqui:master Nov 29, 2023
2 checks passed
jonesde added a commit that referenced this pull request Nov 29, 2023
… a compile error was missed in the changes for PR #625, fix small issues from this
gagaboy added a commit to gagaboy/moqui-framework that referenced this pull request Dec 2, 2023
* commit 'b4287f1b461398b3f22fe96cc9c9761b693a5c07':
  Change EntityDataWriterImpl to use groovy CompileStatic, without this a compile error was missed in the changes for PR moqui#625, fix small issues from this
  Implemented withCloseable/try-with-resources where needed in the code… (moqui#625)
  BugFix EntityListIterator not closed in NotificationMessageImpl#getNotifyUserIds
  Add AutoCloseable extension to EntityListIterator for use with try with resources, thanks to Deepak Dixit for the suggestion
  In addons.xml, added new moqui-sso component.
  In L10nFacadeImpl.formatCurrency() use disableAuthz() for entity find on Uom; small change to currency formatting test to pass with current OOTB settings
  Allow for 10 second threshold in nowTimestamp
  A couple of minor bug fixes in the EntityAutoServiceRunner and ContextJavaUtil (moqui#618)
  Add and Handle Hmac Sha256 with timestamp
  Currency (moqui#614)
  In build.gradle gitStatusAll task also handle upstream remotes with no master branch
  Update various libraries including Groovy to 3.0.19 (which has some minor non-backward compatible changes single 3.0.10 with odd boolean behavior in rare cases, adjusted for in the framework long ago but should be watched for in custom code), Jetty to 10.0.16, H2 database, SLF4J, SnakeYAML, Apache Commons Lang3
  Add text-area.@autogrow attribute, supported only in qvt for now
  In root build.gradle change gitStatusAll task to be more tolerant of repos with no master branch
  In ScreenRenderImpl change addFormFieldValue() and related methods to handle first, second, and last rows, for qvt and other client rendered output that needs full data for a form in a map/object
  Library updates, including Jetty 10.0.13 to 10.015 which had reported vulnerabilities; there are lots of dependencies updated in this set, see diff for full details
  Docker-Image-Pull Feature (moqui#553)
  Add subscreensItem.menuInclude to menu data (moqui#600)
  Fix regression with partitioned tables in PostgreSQL
dixitdeepak added a commit to hotwax/moqui-framework that referenced this pull request Dec 3, 2023
* Fix regression with partitioned tables in PostgreSQL

PostgreSQL JDBC Driver introduced separating type for partitioned table from  40.2.12 pgjdbc/pgjdbc#1708

* Add subscreensItem.menuInclude to menu data (moqui#600)

* Fixed the problem that moqui cannot be deployed as non-root webapp in Tomcat

* Fixed a runtime error if Currency is BTC

* Fixed a runtime error if Currency is BTC

* Fixed the retries of Elastic Client

* Add subscreensItem.menuInclude to menu data

* Docker-Image-Pull Feature (moqui#553)

* Improvement: In Moqui-Multi-Instance added a Async-Pull-Image-Feature which pulls the image by Using Docker-Engine-Api from multiple registry like AWS, Azure, Docker-Hub

* Update AUTHORS

* Added: added the generic way to process cmd , by adding an extra field(authTokenPass) inside the entity InstanceImage , now user has separate field for cmd and password so all types of registries is easily configurable

* Update ServerEntities.xml by adding description

* Library updates, including Jetty 10.0.13 to 10.015 which had reported vulnerabilities; there are lots of dependencies updated in this set, see diff for full details

* In ScreenRenderImpl change addFormFieldValue() and related methods to handle first, second, and last rows, for qvt and other client rendered output that needs full data for a form in a map/object

* In root build.gradle change gitStatusAll task to be more tolerant of repos with no master branch

* Add text-area.@autogrow attribute, supported only in qvt for now

* Update various libraries including Groovy to 3.0.19 (which has some minor non-backward compatible changes single 3.0.10 with odd boolean behavior in rare cases, adjusted for in the framework long ago but should be watched for in custom code), Jetty to 10.0.16, H2 database, SLF4J, SnakeYAML, Apache Commons Lang3

* In build.gradle gitStatusAll task also handle upstream remotes with no master branch

* Currency (moqui#614)

* Use moqui.basic.Uom entity to determine currency formatting and rounding details

* Add currency-hide-symbol attribute as a complement to currency-unit-field, displaying the value without the currency symbol

* Update authors file

* Add and Handle Hmac Sha256 with timestamp

* A couple of minor bug fixes in the EntityAutoServiceRunner and ContextJavaUtil (moqui#618)

* In EntityAutoServiceRunner, remove unwanted break statement to ensure support for multiple PK fields with wildcard (*).

* In ContextJavaUtil, add missing future keyword.

* Updated authors file.

* Allow for 10 second threshold in nowTimestamp

* In L10nFacadeImpl.formatCurrency() use disableAuthz() for entity find on Uom; small change to currency formatting test to pass with current OOTB settings

* In addons.xml, added new moqui-sso component.

* Add AutoCloseable extension to EntityListIterator for use with try with resources, thanks to Deepak Dixit for the suggestion

* BugFix EntityListIterator not closed in NotificationMessageImpl#getNotifyUserIds

* Implemented withCloseable/try-with-resources where needed in the code… (moqui#625)

* Implemented withCloseable/try-with-resources where needed in the code to ensure proper closure of the entity list iterator resource

* Fixed withCloseable syntax

---------

Co-authored-by: David E. Jones <[email protected]>

* Change EntityDataWriterImpl to use groovy CompileStatic, without this a compile error was missed in the changes for PR moqui#625, fix small issues from this

---------

Co-authored-by: Yao Chunlin <[email protected]>
Co-authored-by: David E. Jones <[email protected]>
Co-authored-by: Wei Zhang <[email protected]>
Co-authored-by: Rohit pawar <[email protected]>
Co-authored-by: Jens Hardings <[email protected]>
Co-authored-by: acetousk <[email protected]>
Co-authored-by: Ayman Abi Abdallah <[email protected]>
dixitdeepak added a commit to hotwax/moqui-framework that referenced this pull request Dec 3, 2023
* Fix regression with partitioned tables in PostgreSQL

PostgreSQL JDBC Driver introduced separating type for partitioned table from  40.2.12 pgjdbc/pgjdbc#1708

* Add subscreensItem.menuInclude to menu data (moqui#600)

* Fixed the problem that moqui cannot be deployed as non-root webapp in Tomcat

* Fixed a runtime error if Currency is BTC

* Fixed a runtime error if Currency is BTC

* Fixed the retries of Elastic Client

* Add subscreensItem.menuInclude to menu data

* Docker-Image-Pull Feature (moqui#553)

* Improvement: In Moqui-Multi-Instance added a Async-Pull-Image-Feature which pulls the image by Using Docker-Engine-Api from multiple registry like AWS, Azure, Docker-Hub

* Update AUTHORS

* Added: added the generic way to process cmd , by adding an extra field(authTokenPass) inside the entity InstanceImage , now user has separate field for cmd and password so all types of registries is easily configurable

* Update ServerEntities.xml by adding description

* Library updates, including Jetty 10.0.13 to 10.015 which had reported vulnerabilities; there are lots of dependencies updated in this set, see diff for full details

* In ScreenRenderImpl change addFormFieldValue() and related methods to handle first, second, and last rows, for qvt and other client rendered output that needs full data for a form in a map/object

* In root build.gradle change gitStatusAll task to be more tolerant of repos with no master branch

* Add text-area.@autogrow attribute, supported only in qvt for now

* Update various libraries including Groovy to 3.0.19 (which has some minor non-backward compatible changes single 3.0.10 with odd boolean behavior in rare cases, adjusted for in the framework long ago but should be watched for in custom code), Jetty to 10.0.16, H2 database, SLF4J, SnakeYAML, Apache Commons Lang3

* In build.gradle gitStatusAll task also handle upstream remotes with no master branch

* Currency (moqui#614)

* Use moqui.basic.Uom entity to determine currency formatting and rounding details

* Add currency-hide-symbol attribute as a complement to currency-unit-field, displaying the value without the currency symbol

* Update authors file

* Add and Handle Hmac Sha256 with timestamp

* A couple of minor bug fixes in the EntityAutoServiceRunner and ContextJavaUtil (moqui#618)

* In EntityAutoServiceRunner, remove unwanted break statement to ensure support for multiple PK fields with wildcard (*).

* In ContextJavaUtil, add missing future keyword.

* Updated authors file.

* Allow for 10 second threshold in nowTimestamp

* In L10nFacadeImpl.formatCurrency() use disableAuthz() for entity find on Uom; small change to currency formatting test to pass with current OOTB settings

* In addons.xml, added new moqui-sso component.

* Add AutoCloseable extension to EntityListIterator for use with try with resources, thanks to Deepak Dixit for the suggestion

* BugFix EntityListIterator not closed in NotificationMessageImpl#getNotifyUserIds

* Implemented withCloseable/try-with-resources where needed in the code… (moqui#625)

* Implemented withCloseable/try-with-resources where needed in the code to ensure proper closure of the entity list iterator resource

* Fixed withCloseable syntax

---------



* Change EntityDataWriterImpl to use groovy CompileStatic, without this a compile error was missed in the changes for PR moqui#625, fix small issues from this

---------

Co-authored-by: Yao Chunlin <[email protected]>
Co-authored-by: David E. Jones <[email protected]>
Co-authored-by: Wei Zhang <[email protected]>
Co-authored-by: Rohit pawar <[email protected]>
Co-authored-by: Jens Hardings <[email protected]>
Co-authored-by: acetousk <[email protected]>
Co-authored-by: Ayman Abi Abdallah <[email protected]>
dixitdeepak added a commit to hotwax/moqui-framework that referenced this pull request Dec 13, 2023
* Master (#11) (#12)

* Fix regression with partitioned tables in PostgreSQL

PostgreSQL JDBC Driver introduced separating type for partitioned table from  40.2.12 pgjdbc/pgjdbc#1708

* Add subscreensItem.menuInclude to menu data (moqui#600)

* Fixed the problem that moqui cannot be deployed as non-root webapp in Tomcat

* Fixed a runtime error if Currency is BTC

* Fixed a runtime error if Currency is BTC

* Fixed the retries of Elastic Client

* Add subscreensItem.menuInclude to menu data

* Docker-Image-Pull Feature (moqui#553)

* Improvement: In Moqui-Multi-Instance added a Async-Pull-Image-Feature which pulls the image by Using Docker-Engine-Api from multiple registry like AWS, Azure, Docker-Hub

* Update AUTHORS

* Added: added the generic way to process cmd , by adding an extra field(authTokenPass) inside the entity InstanceImage , now user has separate field for cmd and password so all types of registries is easily configurable

* Update ServerEntities.xml by adding description

* Library updates, including Jetty 10.0.13 to 10.015 which had reported vulnerabilities; there are lots of dependencies updated in this set, see diff for full details

* In ScreenRenderImpl change addFormFieldValue() and related methods to handle first, second, and last rows, for qvt and other client rendered output that needs full data for a form in a map/object

* In root build.gradle change gitStatusAll task to be more tolerant of repos with no master branch

* Add text-area.@autogrow attribute, supported only in qvt for now

* Update various libraries including Groovy to 3.0.19 (which has some minor non-backward compatible changes single 3.0.10 with odd boolean behavior in rare cases, adjusted for in the framework long ago but should be watched for in custom code), Jetty to 10.0.16, H2 database, SLF4J, SnakeYAML, Apache Commons Lang3

* In build.gradle gitStatusAll task also handle upstream remotes with no master branch

* Currency (moqui#614)

* Use moqui.basic.Uom entity to determine currency formatting and rounding details

* Add currency-hide-symbol attribute as a complement to currency-unit-field, displaying the value without the currency symbol

* Update authors file

* Add and Handle Hmac Sha256 with timestamp

* A couple of minor bug fixes in the EntityAutoServiceRunner and ContextJavaUtil (moqui#618)

* In EntityAutoServiceRunner, remove unwanted break statement to ensure support for multiple PK fields with wildcard (*).

* In ContextJavaUtil, add missing future keyword.

* Updated authors file.

* Allow for 10 second threshold in nowTimestamp

* In L10nFacadeImpl.formatCurrency() use disableAuthz() for entity find on Uom; small change to currency formatting test to pass with current OOTB settings

* In addons.xml, added new moqui-sso component.

* Add AutoCloseable extension to EntityListIterator for use with try with resources, thanks to Deepak Dixit for the suggestion

* BugFix EntityListIterator not closed in NotificationMessageImpl#getNotifyUserIds

* Implemented withCloseable/try-with-resources where needed in the code… (moqui#625)

* Implemented withCloseable/try-with-resources where needed in the code to ensure proper closure of the entity list iterator resource

* Fixed withCloseable syntax

---------



* Change EntityDataWriterImpl to use groovy CompileStatic, without this a compile error was missed in the changes for PR moqui#625, fix small issues from this

---------

Co-authored-by: Yao Chunlin <[email protected]>
Co-authored-by: David E. Jones <[email protected]>
Co-authored-by: Wei Zhang <[email protected]>
Co-authored-by: Rohit pawar <[email protected]>
Co-authored-by: Jens Hardings <[email protected]>
Co-authored-by: acetousk <[email protected]>
Co-authored-by: Ayman Abi Abdallah <[email protected]>

* Add sentDate field to WikiBlogCategory entity

---------

Co-authored-by: Yao Chunlin <[email protected]>
Co-authored-by: David E. Jones <[email protected]>
Co-authored-by: Wei Zhang <[email protected]>
Co-authored-by: Rohit pawar <[email protected]>
Co-authored-by: Jens Hardings <[email protected]>
Co-authored-by: acetousk <[email protected]>
Co-authored-by: Ayman Abi Abdallah <[email protected]>
dixitdeepak added a commit to hotwax/moqui-framework that referenced this pull request Aug 20, 2024
* Fix regression with partitioned tables in PostgreSQL

PostgreSQL JDBC Driver introduced separating type for partitioned table from  40.2.12 pgjdbc/pgjdbc#1708

* Add subscreensItem.menuInclude to menu data (moqui#600)

* Fixed the problem that moqui cannot be deployed as non-root webapp in Tomcat

* Fixed a runtime error if Currency is BTC

* Fixed a runtime error if Currency is BTC

* Fixed the retries of Elastic Client

* Add subscreensItem.menuInclude to menu data

* Docker-Image-Pull Feature (moqui#553)

* Improvement: In Moqui-Multi-Instance added a Async-Pull-Image-Feature which pulls the image by Using Docker-Engine-Api from multiple registry like AWS, Azure, Docker-Hub

* Update AUTHORS

* Added: added the generic way to process cmd , by adding an extra field(authTokenPass) inside the entity InstanceImage , now user has separate field for cmd and password so all types of registries is easily configurable

* Update ServerEntities.xml by adding description

* Library updates, including Jetty 10.0.13 to 10.015 which had reported vulnerabilities; there are lots of dependencies updated in this set, see diff for full details

* In ScreenRenderImpl change addFormFieldValue() and related methods to handle first, second, and last rows, for qvt and other client rendered output that needs full data for a form in a map/object

* In root build.gradle change gitStatusAll task to be more tolerant of repos with no master branch

* Add text-area.@autogrow attribute, supported only in qvt for now

* Update various libraries including Groovy to 3.0.19 (which has some minor non-backward compatible changes single 3.0.10 with odd boolean behavior in rare cases, adjusted for in the framework long ago but should be watched for in custom code), Jetty to 10.0.16, H2 database, SLF4J, SnakeYAML, Apache Commons Lang3

* In build.gradle gitStatusAll task also handle upstream remotes with no master branch

* Currency (moqui#614)

* Use moqui.basic.Uom entity to determine currency formatting and rounding details

* Add currency-hide-symbol attribute as a complement to currency-unit-field, displaying the value without the currency symbol

* Update authors file

* Add and Handle Hmac Sha256 with timestamp

* A couple of minor bug fixes in the EntityAutoServiceRunner and ContextJavaUtil (moqui#618)

* In EntityAutoServiceRunner, remove unwanted break statement to ensure support for multiple PK fields with wildcard (*).

* In ContextJavaUtil, add missing future keyword.

* Updated authors file.

* Allow for 10 second threshold in nowTimestamp

* In L10nFacadeImpl.formatCurrency() use disableAuthz() for entity find on Uom; small change to currency formatting test to pass with current OOTB settings

* In addons.xml, added new moqui-sso component.

* Add AutoCloseable extension to EntityListIterator for use with try with resources, thanks to Deepak Dixit for the suggestion

* BugFix EntityListIterator not closed in NotificationMessageImpl#getNotifyUserIds

* Implemented withCloseable/try-with-resources where needed in the code… (moqui#625)

* Implemented withCloseable/try-with-resources where needed in the code to ensure proper closure of the entity list iterator resource

* Fixed withCloseable syntax

---------



* Change EntityDataWriterImpl to use groovy CompileStatic, without this a compile error was missed in the changes for PR moqui#625, fix small issues from this

---------

Co-authored-by: Yao Chunlin <[email protected]>
Co-authored-by: David E. Jones <[email protected]>
Co-authored-by: Wei Zhang <[email protected]>
Co-authored-by: Rohit pawar <[email protected]>
Co-authored-by: Jens Hardings <[email protected]>
Co-authored-by: acetousk <[email protected]>
Co-authored-by: Ayman Abi Abdallah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants