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

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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