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

[Xuelei.Fan at Oracle.Com]

I found the bug when I was learning the t-w-r feature.  But just as Stuart thoroughly analysis, I failed to find a easy way to use t-w-r for these two updates.

Thanks for all of the feedback.

Xuelei

On Jun 29, 2011, at 6:44 AM, Brad Wetmore <bradford.wetmore at oracle.com> wrote:

> 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