[imaging] Closing stream

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[imaging] Closing stream

garydgregory
Hi All:

I see a log of this pattern:

            try {
                if (outputStream != null) {
                    outputStream.close();
                }
            } catch (final Exception e) {
                Debug.debug(e);
            }

for example in org.apache.commons.imaging.Imaging:

    public static void writeImage(final BufferedImage src, final File file,
            final ImageFormat format, final Map<String,Object> params)
throws ImageWriteException,
            IOException {
        OutputStream os = null;

        try {
            os = new FileOutputStream(file);
            os = new BufferedOutputStream(os);

            writeImage(src, os, format, params);
        } finally {
            try {
                if (os != null) {
                    os.close();
                }
            } catch (final Exception e) {
                Debug.debug(e);
            }
        }
    }

Which seems wrong to me. If I get an IOE while writing, we throw, but if we
get one when flushing and closing the file (which may also write), we
swallow it. Why? It seems the IOE from close should percolate up and not be
swallowed.

All of this is moot in Java 7 with try-with-resources blocks but we are not
ready for Java 7 here I imagine.

Gary
--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

sebb-2-2
On 24 October 2013 01:16, Gary Gregory <[hidden email]> wrote:

> Hi All:
>
> I see a log of this pattern:
>
>             try {
>                 if (outputStream != null) {
>                     outputStream.close();
>                 }
>             } catch (final Exception e) {
>                 Debug.debug(e);
>             }
>
> for example in org.apache.commons.imaging.Imaging:
>
>     public static void writeImage(final BufferedImage src, final File file,
>             final ImageFormat format, final Map<String,Object> params)
> throws ImageWriteException,
>             IOException {
>         OutputStream os = null;
>
>         try {
>             os = new FileOutputStream(file);
>             os = new BufferedOutputStream(os);
>
>             writeImage(src, os, format, params);
>         } finally {
>             try {
>                 if (os != null) {
>                     os.close();
>                 }
>             } catch (final Exception e) {
>                 Debug.debug(e);
>             }
>         }
>     }
>
> Which seems wrong to me. If I get an IOE while writing, we throw, but if we
> get one when flushing and closing the file (which may also write), we
> swallow it. Why? It seems the IOE from close should percolate up and not be
> swallowed.

I agree that's bad practice - not only because of swallowing the Exception.
The catch block should catch IOException not Exception.

> All of this is moot in Java 7 with try-with-resources blocks but we are not
> ready for Java 7 here I imagine.
>
> Gary
> --
> E-Mail: [hidden email] | [hidden email]
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

garydgregory
On Wed, Oct 23, 2013 at 9:38 PM, sebb <[hidden email]> wrote:

> On 24 October 2013 01:16, Gary Gregory <[hidden email]> wrote:
> > Hi All:
> >
> > I see a log of this pattern:
> >
> >             try {
> >                 if (outputStream != null) {
> >                     outputStream.close();
> >                 }
> >             } catch (final Exception e) {
> >                 Debug.debug(e);
> >             }
> >
> > for example in org.apache.commons.imaging.Imaging:
> >
> >     public static void writeImage(final BufferedImage src, final File
> file,
> >             final ImageFormat format, final Map<String,Object> params)
> > throws ImageWriteException,
> >             IOException {
> >         OutputStream os = null;
> >
> >         try {
> >             os = new FileOutputStream(file);
> >             os = new BufferedOutputStream(os);
> >
> >             writeImage(src, os, format, params);
> >         } finally {
> >             try {
> >                 if (os != null) {
> >                     os.close();
> >                 }
> >             } catch (final Exception e) {
> >                 Debug.debug(e);
> >             }
> >         }
> >     }
> >
> > Which seems wrong to me. If I get an IOE while writing, we throw, but if
> we
> > get one when flushing and closing the file (which may also write), we
> > swallow it. Why? It seems the IOE from close should percolate up and not
> be
> > swallowed.
>
> I agree that's bad practice - not only because of swallowing the Exception.
> The catch block should catch IOException not Exception.
>

I do not think we should even catch the IOE, it should percolate up, just
like the writeImage call. Why not simply:

{
        final OutputStream os = new BufferedOutputStream(new
FileOutputStream(file));
        try {
            writeImage(src, os, format, params);
        } finally {
            os.close();
        }
    }

Or if your are really paranoid:

{
        OutputStream os = new FileOutputStream(file);
        try {
            os = new BufferedOutputStream(os);
            writeImage(src, os, format, params);
        } finally {
            os.close();
        }
 }

I'm looking for a pattern we can apply throughout [imaging] before 1.0.

Gary


> > All of this is moot in Java 7 with try-with-resources blocks but we are
> not
> > ready for Java 7 here I imagine.
> >
> > Gary
> > --
> > E-Mail: [hidden email] | [hidden email]
> > Java Persistence with Hibernate, Second Edition<
> http://www.manning.com/bauer3/>
> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> > Spring Batch in Action <http://www.manning.com/templier/>
> > Blog: http://garygregory.wordpress.com
> > Home: http://garygregory.com/
> > Tweet! http://twitter.com/GaryGregory
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

sebb-2-2
On 24 October 2013 02:49, Gary Gregory <[hidden email]> wrote:

> On Wed, Oct 23, 2013 at 9:38 PM, sebb <[hidden email]> wrote:
>
>> On 24 October 2013 01:16, Gary Gregory <[hidden email]> wrote:
>> > Hi All:
>> >
>> > I see a log of this pattern:
>> >
>> >             try {
>> >                 if (outputStream != null) {
>> >                     outputStream.close();
>> >                 }
>> >             } catch (final Exception e) {
>> >                 Debug.debug(e);
>> >             }
>> >
>> > for example in org.apache.commons.imaging.Imaging:
>> >
>> >     public static void writeImage(final BufferedImage src, final File
>> file,
>> >             final ImageFormat format, final Map<String,Object> params)
>> > throws ImageWriteException,
>> >             IOException {
>> >         OutputStream os = null;
>> >
>> >         try {
>> >             os = new FileOutputStream(file);
>> >             os = new BufferedOutputStream(os);
>> >
>> >             writeImage(src, os, format, params);
>> >         } finally {
>> >             try {
>> >                 if (os != null) {
>> >                     os.close();
>> >                 }
>> >             } catch (final Exception e) {
>> >                 Debug.debug(e);
>> >             }
>> >         }
>> >     }
>> >
>> > Which seems wrong to me. If I get an IOE while writing, we throw, but if
>> we
>> > get one when flushing and closing the file (which may also write), we
>> > swallow it. Why? It seems the IOE from close should percolate up and not
>> be
>> > swallowed.
>>
>> I agree that's bad practice - not only because of swallowing the Exception.
>> The catch block should catch IOException not Exception.
>>
>
> I do not think we should even catch the IOE, it should percolate up, just
> like the writeImage call. Why not simply:
>
> {
>         final OutputStream os = new BufferedOutputStream(new
> FileOutputStream(file));
>         try {
>             writeImage(src, os, format, params);
>         } finally {
>             os.close();
>         }
>     }

OK by me.
I don't like the Debug.debug(e) statements; does not see appropriate
for a library.

> Or if your are really paranoid:
>
> {
>         OutputStream os = new FileOutputStream(file);
>         try {
>             os = new BufferedOutputStream(os);

I don't like the reuse of the "os" variable here.

>             writeImage(src, os, format, params);
>         } finally {
>             os.close();
>         }
>  }
>
> I'm looking for a pattern we can apply throughout [imaging] before 1.0.
>
> Gary
>
>
>> > All of this is moot in Java 7 with try-with-resources blocks but we are
>> not
>> > ready for Java 7 here I imagine.
>> >
>> > Gary
>> > --
>> > E-Mail: [hidden email] | [hidden email]
>> > Java Persistence with Hibernate, Second Edition<
>> http://www.manning.com/bauer3/>
>> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> > Spring Batch in Action <http://www.manning.com/templier/>
>> > Blog: http://garygregory.wordpress.com
>> > Home: http://garygregory.com/
>> > Tweet! http://twitter.com/GaryGregory
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
>
> --
> E-Mail: [hidden email] | [hidden email]
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Bernd Eckenfels
In reply to this post by garydgregory
Am 24.10.2013, 02:16 Uhr, schrieb Gary Gregory <[hidden email]>:
>             try {
>                 if (outputStream != null) {
>                     outputStream.close();
>                 }
>             } catch (final Exception e) {
>                 Debug.debug(e);
>             }

this calls for a helper or a private method as soon as the code happens  
more than one time in a class IMHO.

> All of this is moot in Java 7 with try-with-resources blocks but we are  
> not
> ready for Java 7 here I imagine.

No, t-w-r is similiar broken to manually closing output streams in catch.  
both syntax constructs need a close (flush is optional) inside the try.  
Some filesystems and abstractions layers do nearly all work or error  
reporting in the close only (nfs, quota, webdav, ...)

Gruss
Bernd
--
http://www.zusammenkunft.net

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Damjan Jovanovic
On Thu, Oct 24, 2013 at 4:34 AM, Bernd Eckenfels <[hidden email]> wrote:

> Am 24.10.2013, 02:16 Uhr, schrieb Gary Gregory <[hidden email]>:
>
>>             try {
>>                 if (outputStream != null) {
>>                     outputStream.close();
>>                 }
>>             } catch (final Exception e) {
>>                 Debug.debug(e);
>>             }
>
>
> this calls for a helper or a private method as soon as the code happens more
> than one time in a class IMHO.
>
>
>> All of this is moot in Java 7 with try-with-resources blocks but we are
>> not
>> ready for Java 7 here I imagine.
>
>
> No, t-w-r is similiar broken to manually closing output streams in catch.
> both syntax constructs need a close (flush is optional) inside the try. Some
> filesystems and abstractions layers do nearly all work or error reporting in
> the close only (nfs, quota, webdav, ...)


Wait, what are you saying is wrong with Java 7's try-with-resources?


> Gruss
> Bernd
> --
> http://www.zusammenkunft.net
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Damjan Jovanovic
In reply to this post by garydgregory
As one of the perpetrators of the problem, I have now fixed it. The
reasons I swallowed exceptions were simple:
* when multiple resources need to be closed, earlier exceptions in
finally cause later close() methods to get skipped and those resources
to leak
* 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

Anyway we now propagate close() exceptions instead of logging and/or
catching, which has shrunk the jar by about 0.5%. The non-leaking with
multiple resources has been preserved but is much uglier:
        } finally {
            IOException closeException = null;
            if (srcChannel != null) {
                try {
                    srcChannel.close();
                } catch (final IOException ioException) {
                    closeException = ioException;
                }
            }
            if (dstChannel != null) {
                try {
                    dstChannel.close();
                } catch (final IOException ioException) {
                    closeException = ioException;
                }
            }
            if (closeException != null) {
                throw closeException;
            }
        }
and original exception propagation benefit will now be lost, but only
Java 7's try-with-resources takes care of all those problems cleanly
anyway (eg. building up a list of all suppressed exceptions inside the
original exception), so you can't have it all.

Damjan

On Thu, Oct 24, 2013 at 3:49 AM, Gary Gregory <[hidden email]> wrote:

> On Wed, Oct 23, 2013 at 9:38 PM, sebb <[hidden email]> wrote:
>
>> On 24 October 2013 01:16, Gary Gregory <[hidden email]> wrote:
>> > Hi All:
>> >
>> > I see a log of this pattern:
>> >
>> >             try {
>> >                 if (outputStream != null) {
>> >                     outputStream.close();
>> >                 }
>> >             } catch (final Exception e) {
>> >                 Debug.debug(e);
>> >             }
>> >
>> > for example in org.apache.commons.imaging.Imaging:
>> >
>> >     public static void writeImage(final BufferedImage src, final File
>> file,
>> >             final ImageFormat format, final Map<String,Object> params)
>> > throws ImageWriteException,
>> >             IOException {
>> >         OutputStream os = null;
>> >
>> >         try {
>> >             os = new FileOutputStream(file);
>> >             os = new BufferedOutputStream(os);
>> >
>> >             writeImage(src, os, format, params);
>> >         } finally {
>> >             try {
>> >                 if (os != null) {
>> >                     os.close();
>> >                 }
>> >             } catch (final Exception e) {
>> >                 Debug.debug(e);
>> >             }
>> >         }
>> >     }
>> >
>> > Which seems wrong to me. If I get an IOE while writing, we throw, but if
>> we
>> > get one when flushing and closing the file (which may also write), we
>> > swallow it. Why? It seems the IOE from close should percolate up and not
>> be
>> > swallowed.
>>
>> I agree that's bad practice - not only because of swallowing the Exception.
>> The catch block should catch IOException not Exception.
>>
>
> I do not think we should even catch the IOE, it should percolate up, just
> like the writeImage call. Why not simply:
>
> {
>         final OutputStream os = new BufferedOutputStream(new
> FileOutputStream(file));
>         try {
>             writeImage(src, os, format, params);
>         } finally {
>             os.close();
>         }
>     }
>
> Or if your are really paranoid:
>
> {
>         OutputStream os = new FileOutputStream(file);
>         try {
>             os = new BufferedOutputStream(os);
>             writeImage(src, os, format, params);
>         } finally {
>             os.close();
>         }
>  }
>
> I'm looking for a pattern we can apply throughout [imaging] before 1.0.
>
> Gary
>
>
>> > All of this is moot in Java 7 with try-with-resources blocks but we are
>> not
>> > ready for Java 7 here I imagine.
>> >
>> > Gary
>> > --
>> > E-Mail: [hidden email] | [hidden email]
>> > Java Persistence with Hibernate, Second Edition<
>> http://www.manning.com/bauer3/>
>> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> > Spring Batch in Action <http://www.manning.com/templier/>
>> > Blog: http://garygregory.wordpress.com
>> > Home: http://garygregory.com/
>> > Tweet! http://twitter.com/GaryGregory
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
>
> --
> E-Mail: [hidden email] | [hidden email]
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Jörg Schaible
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!

- Jörg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

garydgregory
On Thu, Oct 24, 2013 at 4:31 PM, Jörg Schaible <[hidden email]>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

>
> - Jörg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Damjan Jovanovic
On Thu, Oct 24, 2013 at 11:29 PM, Gary Gregory <[hidden email]> wrote:

> On Thu, Oct 24, 2013 at 4:31 PM, Jörg Schaible <[hidden email]>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...

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...

3. Java 7 + try-with-resources, which will cripple portability to Android...

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Jörg Schaible-3
Hi Damjan,

Damjan Jovanovic wrote:

> On Thu, Oct 24, 2013 at 11:29 PM, Gary Gregory <[hidden email]>
> wrote:
>> On Thu, Oct 24, 2013 at 4:31 PM, Jörg Schaible
>> <[hidden email]>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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Julien Aymé
Hi,

Concerning Java 7, I think that the try-with-ressources throws the
first exception encountered, and add other exceptions in the
"suppressed" exceptions.
See http://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html#addSuppressed(java.lang.Throwable)

My 2 cents,
Regards,
Julien

2013/10/25 Jörg Schaible <[hidden email]>:

> Hi Damjan,
>
> Damjan Jovanovic wrote:
>
>> On Thu, Oct 24, 2013 at 11:29 PM, Gary Gregory <[hidden email]>
>> wrote:
>>> On Thu, Oct 24, 2013 at 4:31 PM, Jörg Schaible
>>> <[hidden email]>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: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Damjan Jovanovic
In reply to this post by Jörg Schaible-3
On Fri, Oct 25, 2013 at 9:01 AM, Jörg Schaible
<[hidden email]> wrote:

> Hi Damjan,
>
> Damjan Jovanovic wrote:
>
>> On Thu, Oct 24, 2013 at 11:29 PM, Gary Gregory <[hidden email]>
>> wrote:
>>> On Thu, Oct 24, 2013 at 4:31 PM, Jörg Schaible
>>> <[hidden email]>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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Jörg Schaible-3
Hi Damjan,

Damjan Jovanovic wrote:

[snip]

Thanks for explanation.

> 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);
> }

I guess the nested try was unintentionally ;-)

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Damjan Jovanovic
On Fri, Oct 25, 2013 at 12:36 PM, Jörg Schaible
<[hidden email]> wrote:

> Hi Damjan,
>
> Damjan Jovanovic wrote:
>
> [snip]
>
> Thanks for explanation.
>
>> 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);
>> }
>
> I guess the nested try was unintentionally ;-)
>
> Cheers,
> Jörg

Well that actually won't work, because the "succeeded = true;" will be
skipped if there is a "return;" in the inner try.

Other than a custom Java compiler, I guess there's no clean way of
doing this in Java < 1.7. There's really only option 2 - with being
careful to always set succeeded correctly on all paths out of the try
block. Almost like releasing memory in C.

Damjan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Matt Benson-2
On Oct 25, 2013 6:30 AM, "Damjan Jovanovic" <[hidden email]> wrote:

>
> On Fri, Oct 25, 2013 at 12:36 PM, Jörg Schaible
> <[hidden email]> wrote:
> > Hi Damjan,
> >
> > Damjan Jovanovic wrote:
> >
> > [snip]
> >
> > Thanks for explanation.
> >
> >> 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);
> >> }
> >
> > I guess the nested try was unintentionally ;-)
> >
> > Cheers,
> > Jörg
>
> Well that actually won't work, because the "succeeded = true;" will be
> skipped if there is a "return;" in the inner try.
>
> Other than a custom Java compiler, I guess there's no clean way of
> doing this in Java < 1.7. There's really only option 2 - with being
> careful to always set succeeded correctly on all paths out of the try
> block. Almost like releasing memory in C.
>

I haven't deeply followed this conversation, but would this be a candidate
for a [weaver] module?

Matt

> Damjan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

sebb-2-2
There was some related discussion here:

https://issues.apache.org/jira/browse/IO-249

On 25 October 2013 12:52, Matt Benson <[hidden email]> wrote:

> On Oct 25, 2013 6:30 AM, "Damjan Jovanovic" <[hidden email]> wrote:
>>
>> On Fri, Oct 25, 2013 at 12:36 PM, Jörg Schaible
>> <[hidden email]> wrote:
>> > Hi Damjan,
>> >
>> > Damjan Jovanovic wrote:
>> >
>> > [snip]
>> >
>> > Thanks for explanation.
>> >
>> >> 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);
>> >> }
>> >
>> > I guess the nested try was unintentionally ;-)
>> >
>> > Cheers,
>> > Jörg
>>
>> Well that actually won't work, because the "succeeded = true;" will be
>> skipped if there is a "return;" in the inner try.
>>
>> Other than a custom Java compiler, I guess there's no clean way of
>> doing this in Java < 1.7. There's really only option 2 - with being
>> careful to always set succeeded correctly on all paths out of the try
>> block. Almost like releasing memory in C.
>>
>
> I haven't deeply followed this conversation, but would this be a candidate
> for a [weaver] module?
>
> Matt
>
>> Damjan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Matt Benson-2
Thanks, Sebastian, but I don't see how the discussion there relates to the
"custom Java compiler" alternative postulated by Damjan, which is what I
was responding to.  Which is not to say it doesn't relate; I may be being
thick.

Matt


On Fri, Oct 25, 2013 at 8:50 AM, sebb <[hidden email]> wrote:

> There was some related discussion here:
>
> https://issues.apache.org/jira/browse/IO-249
>
> On 25 October 2013 12:52, Matt Benson <[hidden email]> wrote:
> > On Oct 25, 2013 6:30 AM, "Damjan Jovanovic" <[hidden email]>
> wrote:
> >>
> >> On Fri, Oct 25, 2013 at 12:36 PM, Jörg Schaible
> >> <[hidden email]> wrote:
> >> > Hi Damjan,
> >> >
> >> > Damjan Jovanovic wrote:
> >> >
> >> > [snip]
> >> >
> >> > Thanks for explanation.
> >> >
> >> >> 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);
> >> >> }
> >> >
> >> > I guess the nested try was unintentionally ;-)
> >> >
> >> > Cheers,
> >> > Jörg
> >>
> >> Well that actually won't work, because the "succeeded = true;" will be
> >> skipped if there is a "return;" in the inner try.
> >>
> >> Other than a custom Java compiler, I guess there's no clean way of
> >> doing this in Java < 1.7. There's really only option 2 - with being
> >> careful to always set succeeded correctly on all paths out of the try
> >> block. Almost like releasing memory in C.
> >>
> >
> > I haven't deeply followed this conversation, but would this be a
> candidate
> > for a [weaver] module?
> >
> > Matt
> >
> >> Damjan
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [hidden email]
> >> For additional commands, e-mail: [hidden email]
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

sebb-2-2
It relates to the general discussion

On 25 October 2013 15:21, Matt Benson <[hidden email]> wrote:

> Thanks, Sebastian, but I don't see how the discussion there relates to the
> "custom Java compiler" alternative postulated by Damjan, which is what I was
> responding to.  Which is not to say it doesn't relate; I may be being thick.
>
> Matt
>
>
> On Fri, Oct 25, 2013 at 8:50 AM, sebb <[hidden email]> wrote:
>>
>> There was some related discussion here:
>>
>> https://issues.apache.org/jira/browse/IO-249
>>
>> On 25 October 2013 12:52, Matt Benson <[hidden email]> wrote:
>> > On Oct 25, 2013 6:30 AM, "Damjan Jovanovic" <[hidden email]>
>> > wrote:
>> >>
>> >> On Fri, Oct 25, 2013 at 12:36 PM, Jörg Schaible
>> >> <[hidden email]> wrote:
>> >> > Hi Damjan,
>> >> >
>> >> > Damjan Jovanovic wrote:
>> >> >
>> >> > [snip]
>> >> >
>> >> > Thanks for explanation.
>> >> >
>> >> >> 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);
>> >> >> }
>> >> >
>> >> > I guess the nested try was unintentionally ;-)
>> >> >
>> >> > Cheers,
>> >> > Jörg
>> >>
>> >> Well that actually won't work, because the "succeeded = true;" will be
>> >> skipped if there is a "return;" in the inner try.
>> >>
>> >> Other than a custom Java compiler, I guess there's no clean way of
>> >> doing this in Java < 1.7. There's really only option 2 - with being
>> >> careful to always set succeeded correctly on all paths out of the try
>> >> block. Almost like releasing memory in C.
>> >>
>> >
>> > I haven't deeply followed this conversation, but would this be a
>> > candidate
>> > for a [weaver] module?
>> >
>> > Matt
>> >
>> >> Damjan
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe, e-mail: [hidden email]
>> >> For additional commands, e-mail: [hidden email]
>> >>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [imaging] Closing stream

Jörg Schaible
In reply to this post by Damjan Jovanovic
Hi Damjan,

Damjan Jovanovic wrote:

> On Fri, Oct 25, 2013 at 12:36 PM, Jörg Schaible
> <[hidden email]> wrote:
>> Hi Damjan,
>>
>> Damjan Jovanovic wrote:
>>
>> [snip]
>>
>> Thanks for explanation.
>>
>>> 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);
>>> }
>>
>> I guess the nested try was unintentionally ;-)
>>
>> Cheers,
>> Jörg
>
> Well that actually won't work, because the "succeeded = true;" will be
> skipped if there is a "return;" in the inner try.

Well, but this has to be done in our code, so we can either change it or set
"succedded = true" before the return as well.

To mimic Java 7, we could also implement:

============= %< ===============
 Throwable t = null;
 try {
 } (catch IOException e) { t = e; throw e; }
 // ... another line for each checked exception
 } (catch RuntimeException e) { t = e; throw e; }
 } (catch Error e) { t = e; throw e; }
 } finally {
   closeSafely(t != null, is);
 }
============= %< ===============

but as commented, we have to add a catch for every checked exception,
anotehr one for RuntimeException and one for Error. The approach with the
succeeded flag seems easier ...

> Other than a custom Java compiler, I guess there's no clean way of
> doing this in Java < 1.7. There's really only option 2 - with being
> careful to always set succeeded correctly on all paths out of the try
> block. Almost like releasing memory in C.

Yep.

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12