RFR 8080225: FileInputStream cleanup should be improved

[Peter Levart]

Hi Roger,

I know I'm late for the comments on this one, but anyway...

I'm looking at JNI part of FileDescriptor. There are now two native 
"close" methods for each FileDescriptor variant (unix/windows). One is 
instance method (close0) and the other is static (cleanupClose0), which 
takes an int fd / long handle argument. close0 delegates to C method 
"fileDescriptorClose", which could be implemented entirely in Java with 
a little help from static cleanupClose0, don't you think? This could be 
a follow-up cleanup effort.

Regards, Peter

On 10/04/17 16:35, Roger Riggs wrote:
> 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
>>>
>>>
>>
>