code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile

[Seán Coffey]

On 30/10/2011 16:33, Alan Bateman wrote:
> On 28/10/2011 19:13, Seán Coffey wrote:
>> This is a second stab at cleaning up the close() and finalize() 
>> methods for  FileInputStream / FileOutputStream / RandomAccessFile 
>> classes so that all parents/referents sharing the same native 
>> FileDescriptor are closed out correctly.
>>
>> With Alan's assistance, we have a better implementation in place 
>> where we avoid the use of counters and instead cycle through a list 
>> of shared closeables when a FileDescriptor is being shared.
>>
>> Bug report (not visible yet) 
>> http://bugs.sun.com/view_bug.do?bug_id=7105952
>>
>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.7105952/
>>
>> regards,
>> Sean.
> Thanks for persevering with this somewhat hairy issue. I think the 
> proposed solution is probably the best approach and it's also very 
> simple/easy to understand. Folks may ask why FileDescriptor isn't 
> keeping weak references to the enclosing streams and that's to keep it 
> simple and avoid the complications that FileInputStream and 
> FileOutputStream finalizers bring (they are specified to close the 
> stream which is what lead to the previously messy solution). For the 
> probably <1% of usages where applications create a stream and then 
> construct a new stream with its FileDescriptor then it just means that 
> the otherwise unreferenced stream will remain reachable while the 
> other stream is reachable.
>
> One thing that I think would be good is to clarify the javadoc on 
> exactly how "sharing" of file descriptors is intended to work. I'm not 
> suggesting you do this as part of this change, but just a reminder 
> that the javadoc needs improvement. Another point is that I think this 
> fix should bake for while before thinking about 7u (I realize your 
> primary interest is 7u but this one clearly needs bake time).
I'll add a comment explaining how the sharing should work. Yes - best to 
have this in jdk8 for a few weeks first before backporting.
>
> On the changes then it's a pity that the additions to FileDescriptor 
> have to be duplicated to both implementations. I think we need to look 
> at going back to one implementation, possibly injecting the field for 
> the handle at build time.
>
> Is closeAll missing other.close? I'm also not sure that the suppressed 
> exception handle is completely right - consider the case that 
> releaser.close fails after the close of at least two other streams 
> fails. In that case I think we want the IOException from 
> releaser.close to be thrown with the IOExcepton from the two streams 
> to be the suppressed exceptions. If I read the code correctly then one 
> of them will be the suppressed exception and in turn this will have 
> the other exceptions as suppressed exceptions.  In practical terms I 
> don't think this is a big deal as the stack trace will have everything.
>
Are you referring to the parent.close() call ? If otherParents is null, 
then we only have one reference to this FileDesriptor - i.e the Stream 
that has called close().

Any exception from releaser.close() becomes the main exception if one is 
seen there. Any exceptions from the earlier closes are then added as 
suppressed. I've run some tests on this and it looked to work as 
expected. (stack trace below)
> Minor nit but shouldn't "closed" be private. Also probably should put 
> the standard comments on attach and closeaAll
yes - I'll make that private and clean up the comments.
>
> The webrev makes it hard to read the changes to the test (did you hg 
> mv or hg rm/add?). I think moving it from etc to FileDescriptor is a 
> good idea and you can probably rename it simply to Sharing.java.
I used hg rm/add. I guess hg mv would have worked too. The testcase has 
been modified to test the new closeAll functionality 
(TestCloseReferentHandling()) - I'll rename as suggested.

Might be a day or two before I get to cleaning this up. A few other 
issues on my work list.

regards,
Sean.
>
> -Alan.
>

v245-sus $/suspool/home/sean/jdk8-tl/jdk/build/solaris-sparc/bin/java SC
java.io.IOException: close0 exception!!
         at java.lang.Throwable.fillInStackTrace(Throwable.java:782)
         at java.lang.Throwable.<init>(Throwable.java:265)
         at java.lang.Exception.<init>(Exception.java:66)
         at java.io.IOException.<init>(IOException.java:58)
         at java.io.FileInputStream$1.close(FileInputStream.java:302)
         at java.io.FileDescriptor.closeAll(FileDescriptor.java:208)
         at java.io.FileInputStream.close(FileInputStream.java:299)
         at SC.TestCloseReferentHandling(SC.java:295)
         at SC.main(SC.java:43)
         Suppressed: java.io.IOException: Bad close operation
                 at 
java.lang.Throwable.fillInStackTrace(Throwable.java:782)
                 at java.lang.Throwable.<init>(Throwable.java:265)
                 at java.lang.Exception.<init>(Exception.java:66)
                 at java.io.IOException.<init>(IOException.java:58)
                 at SC$BadFileInputStream.close(SC.java:358)
                 at 
java.io.FileDescriptor.closeAll(FileDescriptor.java:198)
                 ... 3 more
                 Suppressed: java.io.IOException: Bad close operation
                         ... 9 more
                 Suppressed: java.io.IOException: Bad close operation
                         ... 9 more