review request for 7021209, use try-with-resources in lang, math, util

[Stuart Marks]

On 2/21/11 7:23 PM, Joe Darcy wrote:
> David Holmes wrote:
>> Hi Stuart,
>>
>> I'm unclear how try-with-resources applies in different cases. For example,
>> here in Package.java:
>>
>>       private static Manifest loadManifest(String fn) {
>>          try (FileInputStream fis = new FileInputStream(fn);
>>               JarInputStream jis = new JarInputStream(fis, false))
>>          {
>>              return jis.getManifest();
>>          } catch (IOException e) {
>>              return null;
>>          }
>>       }
>>
>> from which code will the the explicit catch block actually catch IOException?
>
> It will catch exceptions from jis.getManifest and from the compiler-generated
> close calls on fis and jis.

It will also catch exceptions from the construction of the FileInputStream and 
the JarInputStream.

>> Does this act as-if it were written:
>>
>>        try {
>>           try (FileInputStream fis = new FileInputStream(fn);
>>                JarInputStream jis = new JarInputStream(fis, false))
>>            {
>>              return jis.getManifest();
>>            }
>>         } catch (IOException e) {
>>             return null;
>>         }
>
> Yes it does.
>
> -Joe

Indeed, since the second block is the initial desugaring of the first, it's 
clear that the catch clause will also catch exceptions thrown from the 
construction of the resources.

David also asked:
> If so it seems a bit redundant to jump through all the suppressed exception stuff only to throw away any exception and return null.

True, some useless suppressed exception code will be generated. However, the 
logic deal with suppressed exceptions will only be executed if there is an 
exception thrown during construction or the jis.getManifest() call, *and* 
another exception is thrown from one of the close() calls on one of the resources.

The primary benefit of using TWR here is to ensure that the FileInputStream 
gets closed properly if construction or processing of the JarInputStream fails, 
which the original code did not do.

(There is also the benefit that the JarInputStream and the FileInputStream are 
both closed properly if getManifest() fails. But there is probably some 
redundancy here, since closing the JarInputStream will also probably close the 
underlying FileInputStream.)

It's an interesting exercise to write out the code with proper exception 
handling, without using TWR, disregarding any handling of suppressed 
exceptions, which we clearly don't care about in this case, and also 
disregarding closing of the JarInputStream. Here's what I came up with:

     private static Manifest loadManifest1(String fn) {
         try {
             Manifest man = null;
             FileInputStream fis = new FileInputStream(fn);
             try {
                 JarInputStream jis = new JarInputStream(fis, false);
                 man = jis.getManifest();
                 jis.close();
             } catch (IOException ioe1) {
                 // ignore
             } finally {
                 try {
                     fis.close();
                 } catch (IOException ioe2) {
                     // ignore
                 }
             }
             return man;
         } catch (FileNotFoundException fnfe) {
             return null;
         }
     }

Quite a mouthful, innit? Somebody could poke holes in this, but I'd guess that 
filling them would make it even more complex. I doubt that this could be 
simplified much if at all.

s'marks