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.
