RFR 8080225: FileInputStream cleanup should be improved

[Roger Riggs]

Hi Mandy,

Updated the webrev in place:
    http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/


On 10/3/2017 7:17 PM, mandy chung wrote:
> Hi Roger,
>
> This looks good overall.
>
> 53 * unreachable should explicitly override {@link #finalize} and call 
> {@code close}.
>
> Since finalize is deprecated, I would not recommend to have the 
> subclass adding the finalize method.  I suggest to take that phrase 
> out (I gave a similar comment to JDK-8185582 [1]). Users should use 
> try-with-resource or register the instances in a cleaner.
ok, but neither t-w-r or cleaner can completely replace the behavior of 
finalize.
In some cases, significant refactoring of the application class may be 
needed.
>
> As for the tests, FinalizeShdCallClose.java implements finalize.  I 
> think it'd be good to convert them to try-with-resource.
The new tests Unreferenced*ClosesFd incorporate the original 
FinalizeShdCallClose tests cases
except the case where the subclass does override finalize to call 
closed; behavior that is deprecated
but still supported.
I refactored the Unreferenced*ClosesFd tests to make the 
FinalizeShdCallClose tests unnecessary and
will remove them. [If the CSR requires greater behavioral compatibility, 
the tests may be needed again].

>
> I'm not close to java.io implementation.  Would registerCleanup be 
> called more than once? line 220 in the registerCleanup will create a 
> new cleanup if it's invoked the second time - is it intentional?
> 216 synchronized void registerCleanup() {
> 217 if (cleanup != null) {
> 218 cleanup.clear();
> 219 }
> 220 cleanup = FDCleanup.create(this);
> 221 }
Re-registering *clears* not cleans the previous cleanup and then creates 
a new one.
If the native fd has changed a new cleanup is needed.
There no cases currently where registerCleanup is called twice on the 
same FileDescriptor.

Thanks, Roger

> thanks Mandy
> [1] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049362.html
>
> On 9/29/17 10:17 AM, Roger Riggs wrote:
>> Replacing finalizers in FileInputStream, FileOutputStream, and adding 
>> cleanup to RandomAccessFile
>> and NIO File Channels with Cleaner based implementations required 
>> changes to FileDescriptor.
>>
>> The specification of FileInputStream and FileOutputStream is changed 
>> to remove the finalizer
>> behavior that required their respective close methods to be called.
>> This change to the behavior is tracked with CSR 8187325 [3].
>>
>> The FileDescriptor implementations (Unix and Windows) provide a 
>> cleanup function that is now used by
>> FIS, FOS, RAF, and async and normal FileChannels.  Each requests 
>> FileDescriptor to register a cleanup function
>> when the fd or handle is assigned and delegates to 
>> FileDescriptor.close.  If the respective
>> FileDescriptor.close method is not called, the fd or handle will be 
>> closed when the FileDescriptor
>> is determined to be phantom reachable.
>>
>> The other uses of FileDescriptor are not intended to be changed, for 
>> example in sockets and datagrams.
>>
>> Some tests were modified to not rely on finalization; new tests are 
>> provided.
>>
>> Comments are appreciated on both the CSR [3] and the implementation [1].
>>
>> [1] webrev: 
>> http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
>>
>> [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
>>
>> [3] CSR:  8187325  FileInputStream/FileOutputStream should use the 
>> Cleaner instead of finalize
>> https://bugs.openjdk.java.net/browse/JDK-8187325
>>
>> Thanks, Roger
>>
>>
>