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.