Code review request: 7082769: FileInputStream/FileOutputStream/RandomAccessFile allow file descriptor be closed when still in use (original) (raw)

Seán Coffey sean.coffey at oracle.com
Fri Sep 9 13:08:04 UTC 2011

Good points Alan.

Coding styles probably differ from merging two testcases together. I'll clean up on the issues mentioned and send out the new webrev shortly.

I thought about try with resources in a few places but it didn't always suit. Take for example the TestMultipleFD() method. The ordering of close call is important. I can get around that knowing that the close calls are made in opposite order to the streams/file's creation. However, the creation of the streams is also important since I take the file descriptor from the random access file (normally) - I might have to intermix try with resources and some try/finally blocks.

regards, Sean.

On 09/09/11 12:25, Alan Bateman wrote:

Seán Coffey wrote:

http://bugs.sun.com/viewbug.do?bugid=7082769

webrev : http://cr.openjdk.java.net/~coffeys/webrev.7082769.7087019.jdk8/ Bug fix where we ensure that the fd object is not disposed of until all streams are closed out. Testcase is a bulked up version of CR 6322678 (which wasn't committed at time of 6322678 fix). It includes create/close() calls for FileInputStream/FileOutputStream/RandomAccessFile which all reference the same file descriptor. Multi threaded access to the same file descriptor is also tested. Typo fix also as per http://bugs.sun.com/viewbug.do?bugid=7087019 also included. I think the change are okay. There are other issues with sharing file descriptors between streams but your changes are a good improvement and eliminate the messy runningFinalizer code (which I think someone added to address a regression from a previous fix to an address another issue with sharing file descriptors). The coverage in the new test looks good but I think the code could be a bit cleaner. For example in TestFinalizer then it uses try-with-resources at L64 but not at L55. It uses /**/ style comments at L63 but // style at L76. Temporary file usage is also inconsistent, Test is created in the system temporary directory, Test6322678 in the working directory. Also the temporary file name is duplicated at L97. I think TestMultipleFD would be a lot cleaner with try-with-resources too. I also suspect the deletes at L138 and L156 may be failing on Windows. If you have time to do a bit of clean-up in the test then I think you are good to go. -Alan.

More information about the core-libs-dev mailing list