Avoid silent closes
-------------------
Key: JCR-1458
URL: https://issues.apache.org/jira/browse/JCR-1458
Project: Jackrabbit
Issue Type: Improvement
Reporter: Jukka Zitting
Priority: Minor
Here's a typical pattern in Jackrabbit code:
OutputStream stream = null;
try {
stream = ...; // create stream
// use stream
} catch (IOException e) {
throw RepositoryException("...", e);
} finally {
if (stream != null) {
try {
stream.close();
} catch (IOException e) {
// ignore
}
}
}
(Note that I recently replaced many of the finally blocks above with a call to
IOUtils.closeQuietly() as part of JCR-1395. The logic is still the same, even
though the number of lines is smaller.)
The assumption is that closing a stream will never throw an exception that
should actually be taken into consideration. After all, you no longer have any
use for the stream, so why care if closing it fails.
However, if you think of scenarios where close() can potentially fail, all of
them are pretty serious issues. Most importantly many OutputStreams buffer data
internally, and the last bytes are only flushed out when close() is called. In
such cases an IOException from close() is just as important as one from
write(). InputStreams are less troublesome, but even there an error in close()
can only mean that something is wrong with the system (otherwise, why would
simple releasing of resources fail).
Thus I would like to get rid of the "silent close" pattern, or at least require
that the rationale for ignoring such exceptions is clearly explained. My
proposal for a better code pattern is:
try {
OutputStream stream = ...; // create stream
try {
// use stream
} finally {
stream.close();
}
} catch (IOException e) {
throw new RepositoryException("...", e);
}
The stacked try blocks add some complexity to the code, but that's the price
you pay for increased reliability. If the extra indenting becomes a problem, I
would just split the inner code to another method that is allowed to throw an
IOException.
Another potential issue is that an exception from close() might end up
shadowing an earlier exception from the try block, but I've never seen that
happen in practice and even if it does happen the end result from the client
perspective is the same: an exception is thrown.
The above reasoning applies also to database and other similar resources. If
the closing of an UPDATE PreparedStatement fails, can we be sure that the
statement was successfully executed? Even the failure in closing a SELECT
ResultSet probably indicates some issue with the database, and I'd rather fail
early with the exception than just ignore it.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
