Hi Joe, thanks for all the info. As I'm not yet familiar with the way how OpenJDK is developed, mind if I ask what steps would have to be taken to add Closeable to javax.naming.Context? The first thing would be to create a patch, test it, run the tests. Then the change has to be proposed, and reviewed. You can argue that this might waste the time of the reviewers with such a small patch. Is there something else that would need to be done? I have read http://openjdk.java.net/contribute/ and signed the OCA, but on the other hand, it's such a small change that most already know how the patch will look. I am not even sure if my time would be spend wisely. The motivation would be to do it to learn how to contribute. Anyway, thank you for your time and consideration. This just some stuff I found, then wrote some code, got some results and did not know what I can or should do with that. Thanks, Johannes On 16-Apr-20 3:15, Joe Darcy wrote:

Hi Johannes,

FYI, back during Project Coin during JDK 7, there were various systematic efforts to review types with a "close" method in the JDK and retrofit AutoCloseable where appropriate:

    JDK-6911261: Project Coin: Retrofit Automatic Resource Management (ARM) support onto platform APIs     https://bugs.openjdk.java.net/browse/JDK-6911261

    JDK-6963723: Project Coin: Retrofit more JDK classes for ARM     https://bugs.openjdk.java.net/browse/JDK-6963723

If you'll excuse the bad import into another blogging system, the process of finding the candidate types with an annotation processor and doing the retrofitting was discussed in a blog entry from that time:

https://blogs.oracle.com/darcy/project-coin:-bringing-it-to-a-closeable

More recently fixed in JDK 14,

    JDK-8203036: com.sun.net.httpserver.HttpExchange should implement AutoCloseable     https://bugs.openjdk.java.net/browse/JDK-8203036

and as of August 2019, closed as will not fix given lack of interest:

    JDK-7057061: Support autocloseable for javax.naming.Context     https://bugs.openjdk.java.net/browse/JDK-7057061

Thanks,

-Joe

On 4/15/2020 5:28 PM, Johannes Kuhn wrote:

...

After failing to wrap a XMLStreamReader in a try-with-resources after discovering it's close() method, I thought about checking what other classes have a public void close() method in the JDK but don't implement AutoCloseable.

So I wrote a small program that enumerates all classes present in in the jrt image with a public void close() method that does not directly or indirectly implement AutoCloseable: https://gist.github.com/DasBrain/8d50e02e35654870e2c2c00bf3f79954 (Output & Code, Code requires JDK 14 with preview features and ASM)

As I am not an expert in any of those areas where classes showed up, and I don't know if it was an oversight, or if there is a good reason for not implementing AutoCloseable. Implementing AutoCloseable is a double edged sword, a few IDEs warn about resource leaks based on that interface. And treat an AutoCloseable passed to methods who return an AutoCloseable as the return value wraping the parameter.

I did look at a few classes: * javax.xml.stream.XMLStreamReader:Javadoc: "Frees any resources associated with this Reader. This method does not close the underlying input source." Better not implement it? * javax.naming.Context: Javadoc: "This method releases this context's resources immediately, instead of waiting for them to be released automatically by the garbage collector." Implement AutoCloseable? * java.util.logging.Handler: Lifecycle seems to be managed by a LogManager. Better not implement AutoCloseable? * jdk.internal.jrtfs.ExplodedImage: class is package private, overrides close() from SystemImage while increasing visibility to public.

I did not restrict my enumeration to public and exported types - it was easier not to do that and I'm lazy.

This touches many unrelated areas, but David Holmes suggested that I take it to core-libs-dev. I know, I just did some static code analysis, but it seems that there might be some low hanging fruits. And for public classes I wish to have at least some rationale why a class with a public void close() method doesn't implement AutoCloseable.

Thanks, Johannes

updated the code, similar output. Disregard org/w3c/dom/html/HTMLDocument https://gist.github.com/DasBrain/df25a69acf564dcb5f2241cc065be2ed About java.xml.stream: A XMLStreamReader is for example constructed with:     BufferedReader br = ...;     XMLStreamReader xsr = XMLInputFactory.newDefaultFactory().createXMLEventReader(br); For my IDE this would look like XMLStreamReader wraps the BufferedReader, so it won't issue a warning if I don't close br. But the Javadoc for XMLStreamReader.close() states:

Frees any resources associated with this Reader. This method does not close the underlying input source.

If those classes would implement AutoCloseable, people and IDEs might wrongly assume that it would close the BufferedReader. Which would result in code like this:     try (var xsr = XMLInputFactory.newDefaultFactory().createXMLEventReader(Files.newBufferedReader(path, StandardCharsets.UTF_8))) {  ... } And have a resource leak. Incidentally, that is the similar to the code I tried to write when I saw that XMLStreamReader has a public void close() method. So in my opinion, to let XMLStreamReader implement AutoCloseable, the specification of the close() method should be changed. The same applies for the other XML Stream/Event Reader/Writer. Thanks, Johannes On 16-Apr-20 9:28, Alan Bateman wrote:

On 16/04/2020 01:28, Johannes Kuhn wrote:

...

:

I did not restrict my enumeration to public and exported types - it was easier not to do that and I'm lazy. Most of the candidates that you've identified are JDK internal classes and there are several non-public classes in exported packages. There are also a few cases where the sub-classes of the likely candidate are showing up too (e.g. all the public sub-classes of java.util.logging.Handler). I skimmed through your list and filtered it down to the public classes in exported packages to following:

com/sun/jdi/connect/spi/Connection java/awt/SplashScreen java/util/logging/Handler javax/naming/Context javax/naming/NamingEnumeration javax/naming/ldap/StartTlsResponse javax/smartcardio/CardChannel javax/sql/PooledConnection javax/swing/ProgressMonitor javax/xml/stream/XMLEventReader javax/xml/stream/XMLEventWriter javax/xml/stream/XMLStreamReader javax/xml/stream/XMLStreamWriter

As Joe points out, some of these may have been looked at already. I was surprised to see the java.xml.stream reader/writers so it might be worth digging into those to see why they were not retrofitted.

-Alan.

On 4/21/20 10:06 PM, Stuart Marks wrote:

(The usual objection to this construct is if closing the wrapper closes the underlying reader, the t-w-r will close it again. This isn't a problem for BufferedReader and most JDK I/O classes, as close() is idempotent for them.)

So here's a Java design question (or two) for you: 1. Should close() always be idempotent, where practical? I would have thought so, but perhaps there are downsides. 2. Should classes which implement close() with the standard meaning be AutoCloseable? -- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Hello, On 4/22/2020 6:12 AM, Alan Bateman wrote:

On 22/04/2020 13:50, Andrew Haley wrote:

...

: 1. Should close() always be idempotent, where practical? I would have     thought so, but perhaps there are downsides.

2. Should classes which implement close() with the standard meaning be     AutoCloseable?

I'm sure Joe Darcy can say more on this but I remember there was a lot of effort put into this topic when AutoCloseable was added in Java 7 (and Project Coin). Closeable close is idempotent but AutoCloseable close could not require it. AutoCloseable's API docs recommend it of course. There was effort in Java 7 and beyond to retrofit existing classes that defined a close method to be Closeable or AutoCloseable. There are only a handful of exported APIs remaining that have a close method that don't extend or implement AutoCloseable. I don't know the history of the XML stream interface to know why they close to define them not to close the underlying stream but I doubt these could be changed now.

Yes, the JSR 334 EG had some discussions about both "SilentCloseable" (no exceptions) and "IdempotentCloseable" as possible library additions back in the JDK 7 time frame. These were not judged to have sufficient marginal utility over Closeable and AutoCloseable to include in the platform. It was impractical to require idempotent close methods in call cases, but they are recommended. Generally, a type with a void close method should implement AutoClosable. Most of the candidate types in the JDK were updated for JDK 7; a few stragglers were updated since then, but there are a few remaining cases that could be updated as discussed earlier in this thread. HTH, -Joe

Hi Johannes, On 16/04/2020 08:28, Alan Bateman wrote:

java/util/logging/Handler

That's already logged as https://bugs.openjdk.java.net/browse/JDK-8025709 (assigned to me) I have never got to the point to start working on it because it seems to be a very niche case: Handler are usually attached to loggers, so unless you're writing a test or using the API in ways that it was not designed for (though they might still be legitimate), then it's very unlikely that you would use a Handler in a try with resource statement. The java.logging module is mainly in maintenance mode these days and any changes we do there have a tendency to ripple out of control and come back to bite us - so we usually tend to exercise caution. best regards, -- daniel

As Joe points out, some of these may have been looked at already. I was surprised to see the java.xml.stream reader/writers so it might be worth digging into those to see why they were not retrofitted.