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

[Seán Coffey]

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/view_bug.do?bug_id=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/view_bug.do?bug_id=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.
>
>