Some Classes with a public void close() don't implement AutoCloseable

[Stuart Marks]

Hi Johannes,

On 4/16/20 3:09 AM, Johannes Kuhn wrote:
> 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.

The typical try-with-resources idiom for this looks like:

     try (var br = new BufferedReader(...);
          var xsr = XMLInputFactory.newDefaultFactory().createXMLEventReader(br)) {
         ...
     }

This should work even though XMLEventReader.close() is specified not to close 
the underlying Reader. It will also work to close the BufferedReader if the 
creation of XMLEventReader throws an exception.

(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.)

> 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.

I'm not sure what to make of what an IDE tells you. If it's making unwarranted 
assumptions about whether XMLEventReader (or some other wrapper) does or does 
not close the wrapped object, then it's a bug.

Note that the nested construction example shown here

     try (var xsr = ....createXMLEventReader(Files.newBufferedReader(...)))

*will* leak the BufferedReader if createXMLEventReader() throws an exception, 
since nobody has kept the reference to the BufferedReader. So in all cases, I 
recommend that people use the multiple variable declaration form of t-w-r.

> So in my opinion, to let XMLStreamReader implement AutoCloseable, the specification of the close() method should be changed.

This would be an incompatible change. There might be code that relies on 
XMLStreamReader not to close the underlying reader, and reuses the reader for 
something.

> The same applies for the other XML Stream/Event Reader/Writer.

On the face of it, making these AutoCloseable seems reasonable to me. However, 
I'm somewhat distant from this area, so I don't know if that makes sense for 
these particular XML interfaces.

s'marks

> 
> 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.
>>
>>
>>
>