Code review request: 7059709, java/jsse, close the IO in a final block

[Brad Wetmore]

While I was thinking the same thing as Sean/Stuart, it's not clear how 
much risk you would be refactoring a lot of code just to use this 
language feature.

It's pretty clear (to me anyway) what you were intending here.

Brad




On 6/28/2011 1:31 PM, Stuart Marks wrote:
> On 6/28/11 11:46 AM, Sean Mullan wrote:
>> On 6/27/11 9:29 PM, Xuelei Fan wrote:
>>> webrev: http://cr.openjdk.java.net/~xuelei/7059709/webrev.00/
>>
>> It seems you could restructure this code by moving the instantiation
>> of the
>> FileInputStream down just before the KeyStore.load and use a
>> try-with-resources
>> block instead.
>
> I had thought of this too but using try-with-resources for either of
> these cases isn't obvious.
>
> One difficulty is that the FileInputStreams are only initialized if some
> condition is true, otherwise they're left null. That is, simplified,
>
> FileInputStream fis = null;
> if (...condition...) {
> fis = ...initialization...
> }
> // process fis if non-null
>
> To use try-with-resources, you'd have to do something like put the
> conditional within the initialization expression. The only way to do
> this in-line in Java is to use the ternary (?:) operator, like so:
>
> try (FileInputStream fis = ...condition... ? ...initialization... : null) {
> // process fis if non-null
> }
>
> or the conditional could be evaluated prior to the try, with the
> resource variable being an alias:
>
> FileInputStream fis = null;
> if (...condition...) {
> fis = ...initialization...
> }
> try (FileInputStream fis2 = fis) {
> // process fis2 if non-null
> }
>
> or even by refactoring the conditional initialization into a separate
> method:
>
> try (FileInputStream fis = openFileOrReturnNull(...)) {
> // process fis if non-null
> }
>
> Another approach would be to call KeyStore.load() from the different
> condition arms, instead of conditionally initializing a variable:
>
> if (...condition...) {
> try (FileInputStream fis = ...initialization...) {
> ks.load(fis, passwd);
> }
> } else {
> ks.load(null, passwd);
> }
>
> This initializes the input streams closer to where they're used, but
> this might change the behavior if an error occurs while opening the
> stream; additional initializations or even side effects might occur
> before the error is reached. I haven't inspected the code thoroughly to
> determine whether this is the case. It also puts big initialization
> expression into the t-w-r, which might be ugly.
>
> Now, as you might know, I'm a big fan of try-with-resources, but using
> it here brings in a potentially large restructuring. This might or might
> not be warranted for this particular change; I don't know the tradeoffs
> involved in this code.
>
> s'marks