commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jörg Schaible <Joerg.Schai...@scalaris.com>
Subject Re: [imaging] Closing stream
Date Fri, 25 Oct 2013 07:01:27 GMT
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 ... :-/


> 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;
   }
 }
=============== %< ==================


> 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


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


Mime
View raw message