RFR 8080225: FileInputStream cleanup should be improved

[Roger Riggs]

Hi Peter,

On 10/14/2017 5:00 AM, Peter Levart wrote:
>
> On 10/14/17 10:51, Peter Levart wrote:
>> 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? 
>
> Well, there is this complication in UNIX variant about closing 0,1 or 
> 2 descriptors. Perhaps the static cleanupClose0 method could 
> encapsulate this logic and have the following signature:
>
> private static native int cleanupClose0(int fd);
>
> The all-Java close0() would then use the returned value and replace it 
> in FileDescriptor.fd.
>
> What do you think?

The native handling of the file descriptor and setting the field to -1 
in native code was,
I think, trying to avoid races in the closing of the fd.  The functions 
in io_util_md.c
are also used for sockets and datagrams in addition to files.  It has 
always been sensitive code.
The new CleanupClose0 was intended only for the case where a FD was 
unreferenced.
The System.in/out/err FileDescriptors should never become unreferenced 
so the redirection
to /dev/null is not needed in that case.

I filed a cleanup bug: https://bugs.openjdk.java.net/browse/JDK-8189330
to take another look later.

Thanks, Roger

>
> Regards, Peter
>
>> 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
>>>>>
>>>>>
>>>>
>>>
>>
>