apache / jena

Apache Jena
https://jena.apache.org/
Apache License 2.0
1.1k stars 647 forks source link

Make org.apache.jena.atlas.lib.Closeable extend java.lang.AutoCloseable #1704

Open a14n opened 1 year ago

a14n commented 1 year ago

Version

4.7.0

Feature

By making org.apache.jena.atlas.lib.Closeable extend java.lang.AutoCloseable it will allow user to use try-with-resources and enable detection by tools of missing .close().

Are you interested in contributing a solution yourself?

None

rvesse commented 1 year ago

So we've discussed this in the past as a project and we've chosen not to do this. Jena is a project with a long history that predates many modern Java features, e.g. AutoCloseable, and with a large user base with widely varying technical literacy. (Aside: Yes AutoCloseable has existed since Java 7 which was released 2011 but Jena started in the very early 2000s i.e. Java 4)

While this may seem like a trivial change, making it will immediately litter every consumer of Jena's codebase's with huge numbers of warnings. Not to mention the internals of Jena itself, because as a set of libraries often we are passing back references to these Closeable objects and expecting API consumers to handle closure suitably.

That's not to say that this change will never happen but it would most likely be part of a major version bump and potentially combined with more wider cleanup of some of the legacy aspects of the codebase

afs commented 1 year ago

@a14n - Which API resources are you referring to?

As @rvesse says, the impact on many Jena API users would be extensive. (Result - they will switch the warnings off and nothing is achieved.) See JENA-1601.

It is also evident that different contributors do not treat try-with-resources in the same way - we get code with several different types of warnings and it turns out contributors have, across their whole dev setup, chosen to switch them off.

Jena can (and does) support different API presentations of the core functionality. The evolutionary approach is to add a new API along side the existing ones.

a14n commented 1 year ago

This issue is indeed a dup of JENA-1601 and my concern was exactly what the description explained.

But I also raised this issue to detect more easily issue like #1670.

afs commented 1 year ago

@a14n - have you observed any effects of not closing partially consumed iterators? The contract is "consume or close (or both)".

Using some iterators across transactions boundaries is illegal and is detected.

a14n commented 1 year ago

have you observed any effects of not closing partially consumed iterators?

Indirectly via #1670 ;)

The contract is "consume or close (or both)".

Sure but humans make mistakes and a warning on unclosed resources would help to avoid forgetting to close partially consumed iterators.

Feel free to close the issue if you don't think it makes sense.

ajs6f commented 1 year ago

I think @rvesse put it well:

That's not to say that this change will never happen but it would most likely be part of a major version bump and potentially combined with more wider cleanup of some of the legacy aspects of the codebase