[
https://issues.apache.org/jira/browse/JCR-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12575631#action_12575631
]
Thomas Mueller commented on JCR-1458:
-------------------------------------
Sorry, correction, your example would not _always_ fail in the stream.close()
method if there is a problem. But it might, and then we can't be sure if this
was actually the problem.
> 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.
