commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Damjan Jovanovic <damjan....@gmail.com>
Subject Re: [imaging] Closing stream
Date Fri, 25 Oct 2013 07:52:54 GMT
On Fri, Oct 25, 2013 at 9:01 AM, Jörg Schaible
<Joerg.Schaible@scalaris.com> wrote:
> Hi Damjan,
>
> Damjan Jovanovic wrote:
>
>> On Thu, Oct 24, 2013 at 11:29 PM, Gary Gregory <garydgregory@gmail.com>
>> wrote:
>>> On Thu, Oct 24, 2013 at 4:31 PM, Jörg Schaible
>>> <joerg.schaible@gmx.de>wrote:
>>>
>>>> Hi Damjan,
>>>>
>>>> Damjan Jovanovic wrote:
>>>>
>>>> > As one of the perpetrators of the problem, I have now fixed it. The
>>>> > reasons I swallowed exceptions were simple:
>>>>
>>>> [snip]
>>>>
>>>> > * when an exception is thrown and close() then throws another
>>>> > exception, the close() exception is propagated and the original
>>>> > exception - which could reveal the cause of the problem - swallowed
>>>>
>>>> [snip]
>>>>
>>>> And this *is* a real problem. And it is exactly why the IOException of
>>>> close() are normally ignored. While I don't like
>>>> IOUtils.closeQietly(closeable), I use normally a method
>>>> "IO.closeLogged(closeable)" resp. "IO.closeLogged(closeable, logger)".
>>>>
>>>> If e.g. an image is corrupted and later on an additional exception
>>>> occurs closing any resource, you will simply *never* learn about the
>>>> corrupted image that caused the problem in first case. The original
>>>> exception must never be swallowed!
>>>>
>>>
>>> Nest'em!
>>>
>>> G
>>
>> What's the way forward though?
>>
>> 1. Catching both exceptions and nesting with setCause():
>>
>> InputStream is = null;
>> Exception ex = null;
>> try {
>>     is = factoryMethodThatCanThrow();
>>     is.methodThatCanThrow();
>> } catch (Exception exx) {
>>     ex = exx;
>> } finally {
>>     if (is != null) {
>>         try {
>>             is.close();
>>         } catch (IOException closeEx) {
>>             if (ex != null) {
>>                 closeEx.setCause(ex);
>>             }
>>             throw closeEx;
>>         }
>>     }
>> }
>>
>> which only gets worse, as each type of exception has to be separately
>> caught and rethrown...
>
>
> Right, you could have got even an OOME reading an image ... :-/

And setCause() isn't really appropriate since the exception thrown by
close() can be causally unrelated to the original exception.

>> 2. Swallowing close() exceptions when a succeeded flag at the end of
>> the try wasn't set:
>>
>> InputStream is = null;
>> boolean succeeded = false;
>> try {
>>     is = factoryMethodThatCanThrow();
>>     is.methodThatCanThrow();
>>     succeeded = true;
>> } finally {
>>     if (is != null) {
>>         try {
>>             is.close();
>>         } catch (IOException closeEx) {
>>             if (succeeded) {
>>                 throw closeEx;
>>             }
>>         }
>>     }
>> }
>>
>> which now also requires making sure you don't "return;" before the end
>> of the try...
>
>
> IMHO this is the best approach so far. Maybe we can introduce a utility
> method somewhere to avoid the boilerplate copies:
>
> =============== %< ==================
>  public static void closeSafely(boolean mayThrow, Closeable... closeables)
> throws IOException {
>    IOException ioex = null;
>    for(Closeable closeable : closeables) {
>      if (closeable != null) {
>        try {
>          closeable.close();
>        } catch (IOException ex) {
>          ioex = ex; // keep first or last ?!?
>        }
>      }
>    }
>    if (mayThrow && ioex != null) {
>       throw ioex;
>    }
>  }
> =============== %< ==================

I like that method, but I'd rather we always set succeeded correctly. Read on.

>> 3. Java 7 + try-with-resources, which will cripple portability to
>> Android...
>
> I would first write a unit test to see what Java 7 actually does with
> multiple open resources in the different error cases.
>
> Cheers,
> Jörg

Java 7 compiles:

try (InputStream is = factoryMethodThatCanThrow()) {
    is.methodThatCanThrow();
}

into:

final InputStream is = factoryMethodThatCanThrow();
Throwable primaryException = null;
try {
    is.methodThatCanThrow();
} catch (Throwable t) {
    primaryException = t;
    throw t;
} finally {
    if (is != null) {
        if (primaryException != null) {
            try {
                is.close();
            } catch (Throwable t) {
                primaryException.addSuppressed(t);
            }
        } else {
            is.close();
        }
    }
}

When try-with-resources has a catch and/or finally, it compiles into 2
nested try blocks, this being the inner, and the one with
catch/finally being the outer. When multiple resources are being
managed by a try-with-resources block, each resource compiles into its
own try-finally block as above, nested in the previous resource's try
body. The innermost try body contains the body of the original
try-with-resources block. For examples see
http://stackoverflow.com/questions/7860137/what-is-the-java-7-try-with-resources-bytecode-equivalent-using-try-catch-finall

We would be able to adapt that for Java < 1.7 by swallowing the close
exception instead of calling addSuppressed() on the primary exception,
but the show stopper is catching and rethrowing the primary exception
(Throwable), which javac < 1.7 refuses to compile because it doesn't
do "Rethrowing Exceptions with More Inclusive Type Checking"
(http://docs.oracle.com/javase/7/docs/technotes/guides/language/catch-multiple.html).

But this would work and always sets succeeded correctly without catch/re-throw:

final InputStream is = factoryMethodThatCanThrow();
boolean succeeded = false;
try {
    try {
        is.methodThatCanThrow();
    } finally {
    }
    succeeded = true;
} finally {
    closeSafely(!succeeded, is);
}

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message