[
https://issues.apache.org/jira/browse/JCR-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12575629#action_12575629
]
Thomas Mueller commented on JCR-1458:
-------------------------------------
Hi,
I agree it's better to not ignore exceptions on close. I think it would be even
worse if we shadow exceptions (ignore other exceptions) however. Your example
would always fail in the stream.close() method; the real reason for the
exception is always shadowed. What about our own RepositoryUtils.close(..)
methods?
RepositoryException RepositoryUtils.convertIOException(String message,
OutputStream out, IOException e) {
IOUtils.closeSilently(out);
return new RepositoryException(message, e);
}
and then use
OutputStream out = ...;
try {
// use stream
out.close();
} catch (IOException e) {
throw RepositoryUtils.convertIOException("...", out, e);
}
This would reduce the boilerplate code, and only silently ignore the exception
on close if there is an IO exception before closing the stream.
> 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.
