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

[Seán Coffey]

Cleaned up testcase (as per suggestions) is in latest webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.7082769.7087019.jdk8.2/

I've kept the try-with-resouces approach out of the TestMultipleFD  
method. It just complicates matters and with the closing order being 
important, it's easier to read with old style.

Regards,
Sean.

On 09/09/2011 14:08, Seán Coffey wrote:
> 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.
>>
>>