jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jukka Zitting (JIRA)" <j...@apache.org>
Subject [jira] Created: (JCR-1458) Avoid silent closes
Date Thu, 06 Mar 2008 09:15:01 GMT
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.


Mime
View raw message