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
