code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
Alan.Bateman at oracle.com
Sun Oct 30 16:33:06 UTC 2011
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)
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.7105952/
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.
More information about the core-libs-dev