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

[Alan Bateman]

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).

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.

Minor nit but shouldn't "closed" be private. Also probably should put 
the standard comments on attach and closeaAll

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.

-Alan.