RFR 8080225: FileInputStream cleanup should be improved
Peter Levart
peter.levart at gmail.com
Sat Oct 14 09:00:25 UTC 2017
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?
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
>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list