moqui / moqui-framework

Use Moqui Framework to build enterprise applications based on Java. It includes tools for databases (relational, graph, document), local and web services, web and other UI with screens and forms, security, file/resource access, scripts, templates, l10n, caching, logging, search, rules, workflow, multi-instance, and integration.
http://www.moqui.org
Other
279 stars 199 forks source link

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

Closed dixitdeepak closed 9 months ago

dixitdeepak commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.