review request for 7021582, use try-with-resources in jar/zip implementation and tests

[Stuart Marks]

[this is a resend of an earlier message, which apparently didn't go through]

On 2/24/11 2:20 AM, Alan Bateman wrote:
> Stuart Marks wrote:
>> Hi Sherman, all,
>>
>> Here's a webrev to convert code in the jar/zip implementation files and tests
>> to use the new Java 7 try-with-resources construct.
>>
>> http://cr.openjdk.java.net/~smarks/reviews/7021582/webrev.0/
> I looked through the src/** changes (I don't have time to go through the test
> changes just now).

Thanks, this is helpful.

> All looks okay to me except for
> src/share/classes/com/sun/java/util/jar/pack/PropMap.java.
>
> At L129 it uses getResourceAsStream and that will return null if the properties
> file doesn't exist causing props.load to throw NPE. I realize the original code
> will NPE too but maybe this should be fixed while you are in the area.

Good point. I'm definitely of the mindset of cleaning up stuff in the same 
area, even unrelated to TWR, if it's reasonably straightforward to do so.

> Also in this code it's not clear to me that propsLoaded is needed. I realize
> you're keeping existing behavior but it means that we get NPE if the properties
> file doesn't exist, throw RuntimeException if there is a problem reading it,
> and do nothing if close fails. I can't think of a case where close could fail
> here and maybe it would be better to just handle it in the same way as an error
> reading the properties.

Yeah, the existing behavior seemed dubious and I didn't like that I had to 
invent a boolean to preserve that behavior. If you think it's OK to change the 
behavior here to handle (instead of ignore) an IOException thrown by close() 
then I'm all for it.

In general whereas it's clear that a failure in close() after writing needs to 
be handled, I'm less clear on whether a failure in close() for something that's 
read-only ought to be handled -- or even what such a failure means. If it 
"never" happens then it's probably not worth special-casing it to make it a 
no-op. Also, if there is an exception thrown from close() even on something 
that's read-only, maybe the system is trying to tell us something ("You know 
those bytes you just read? Well, they were wrong.") so we ought to handle the 
exception instead of ignoring it. It's hard to imagine this happening, but just 
because it's hard to imagine doesn't mean that it won't happen.

OK, I think I've convinced myself that one should never ignore an exception 
from close() even on something read-only. With TWR there's no point in doing 
so. You already have a try-block that does the processing; it implicitly 
includes the close() now. It either has a catch of IOException, or there is a 
"throws IOException" on the enclosing method. So IOException should already be 
handled properly, so there's nothing to be gained by special-casing IOException 
from close().

s'marks