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

[Peter Levart]

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
>
>
>
>