Fwd: Re: RFR 8080225 FileInput/OutputStream/FileChannel cleanup should be improved

[Roger Riggs]

Hi Peter,

I'd like to keep the code recognizably simple; (in the absence of a 
known performance issue).
It would be interesting to know how the streams versions compares to the 
exception versions.
Something for a rainy day...

Roger


On 12/4/2017 8:29 AM, Peter Levart wrote:
> Sending previous message to the list also (forgot to mention it in CC)...
>
> Hi Rogger,
>
> Interesting approach. Conditional finalization. You use finalization to
> support cases where user overrides finalize() and/or close() and Cleaner
> when he doesn't.
>
> I wonder if it is the right thing to use AltFinalizer when user
> overrides finalize() method. In that case the method is probably not
> empty and calls super.finalize() (if it is empty or doesn't call super,
> user probably doesn't want the finalization to close the stream) and so
> normal finalization applies. If you register AltFinalizer for such case,
> close() will be called twice.
>
> The logic should probably be 3-state:
>
> - if user overrides finalize() - do nothing; else
> - if user overrides close() - use AltFinalizer; else
> - use Cleaner
>
> Some additional comments:
>
> - FileInputStream.AltFinalizer#get could be written to not use
> exceptions for flow control. For example:
>
>     static AltFinalizer get(FileInputStream fis) {
>         return Stream.<Class<?>>iterate(fis.getClass(),
>                                         cl -> cl != 
> FileInputStream.class,
>                                         Class::getSuperclass)
>             .flatMap(clazz -> Stream.of(clazz.getDeclaredMethods()))
>             .filter(m -> m.getParameterCount() == 0 &&
>                          (m.getName().equals("close") ||
>                           m.getName().equals("finalize")))
>             .findFirst()
>             .map(m -> new AltFinalizer(fis)).orElse(null);
>     }
>
> or, to support 3-state logic:
>
>     static Optional<AltFinalizer> get(FileInputStream fis) {
>         int flag = Stream.
>             <Class<?>>iterate(fis.getClass(),
>                               cl -> cl != FileInputStream.class,
>                               Class::getSuperclass)
>             .flatMap(clazz -> Stream.of(clazz.getDeclaredMethods()))
>             .mapToInt(m -> m.getParameterCount() == 0
>                            ? m.getName().equals("close")
>                              ? 1
>                              : (m.getName().equals("finalize") ? 2 : 0)
>                            : 0)
>             .reduce(0, (i, j) -> i | j);
>
>         if ((flag & 2) != 0) { // overrides finalize()
>             return Optional.empty();
>         } else if ((flag & 1) != 0) { // overrides close()
>             return Optional.of(new AltFinalizer(fis));
>         } else { // overrides none
>             return null;
>         }
>     }
>
> - similar for FileOutputStream.AltFinalizer
>
>
> That's all for now. Will try to check the rest later.
>
> Regards, Peter
>
> On 12/02/2017 03:38 AM, Roger Riggs wrote:
>> Please review a revision to the work on remove a dependency on 
>> finalization from
>> FileInputStream, FileOutputStream, and add cleanup of closing file 
>> descriptors in FileChannel.
>>
>> The previous version was too aggressive in removing the finalize 
>> method and was considered to be insufficiently
>> backward compatible.
>>
>> For compatibility with previous versions, the close method will be 
>> called when the FIS/FOS is unreachable
>> only when FIS/FOS has been subclassed to override close() or 
>> finalize().  In other cases, the cleaner is used
>> to make sure the FileDescriptor is closed.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8080225
>>
>> CSR:
>> Relax FileInputStream/FileOutputStream requirement to use finalize
>> https://bugs.openjdk.java.net/browse/JDK-8187325
>>
>> Thanks, Roger
>>
>>
>>
>>
>