review request for 7021582, use try-with-resources in jar/zip implementation and tests
stuart.marks at oracle.com
Thu Feb 24 23:35:21 UTC 2011
[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.
> 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
> 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
More information about the core-libs-dev