search

eXist-db / exist

eXist Native XML Database and Application Platform
https://exist-db.org
GNU Lesser General Public License v2.1
424 stars 179 forks source link

[6.x.x] Removed finalize() methods #5323

Open reinhapa opened 4 months ago

reinhapa commented 4 months ago

Removed all finalize() methods for the following classes:

Remplaced finalize() method with shutdown hook:

Description:

Fixes a SAXParseException that occured in heavy use denoting that the document does missing a needed ending tag or having a premature end of file.

dizzzz commented 4 months ago

From what I understand , this is in essence a backport of v7?

adamretter commented 4 months ago

this is in essence a backport of v7?

@dizzzz Does that mean the problems I mentioned above also present in v7?

dizzzz commented 4 months ago

Yes; That is what I remember, and Patrick confirmed this

reinhapa commented 4 months ago

this is in essence a backport of v7?

@dizzzz Does that mean the problems I mentioned above also present in v7?

Not all issues are present the same way as the Resource interface now also extends the AutoCloseable and the issues in the v7 code base have been addressed when switching to the current XMLDB::API version so far. The only thing missing would be to add AutoCloseable also to the ResourceSet interface.

I have digged into the described issue of stuncated resources at least to days now and it seems the the JVM will actually prematurely call the finalize() method on Objects, that are still referred in our case some times (only on the client side useage so far I can see it. I could solve it for my usage by storing the created Remote[XML/Binary]Resource instances within the RemoteResourceSet so far and closing those as the clear() method is called.

For the TemporaryFileManager I already switched to the better NIO cleanup mechanism by using the DELETE_ON_CLOSE open option. Here we could also look into the Cleaner API instead using the finalize() method in the future...

The AbstractCachedResult I have checked in v7 so far but here we should also look into the Cleaner API .

@adamretter all in all it would be great to have a in person discussion to see how we should takle the finalize() problem once and for all in order to be ready to move up in the new JDKs in the future...

  1. InputSource is already highly problematic in its design. Historically classes that use this (including inside and outside of eXist-db code, e.g. Xerces, etc.) have not been good at closing the resources they get from calling InputSource#getByteStream() or InputSource#getReader(). The JavaDoc states:

An InputSource object belongs to the application: the SAX parser shall never modify it in any way (it may modify a copy if necessary). However, standard processing of both byte and character streams is to close them on as part of end-of-parse cleanup, so applications should not attempt to re-use such streams after they have been handed to a parser.

As far as I understand the documentation the parsing instance is in charge to close potentially open streams at the end of the parse operation. That would mean that we need to do it or make sure that it is being done, right?

dizzzz commented 1 month ago

Please can you discuss this? @adamretter @reinhapa