RFR 8080225 FileInput/OutputStream/FileChannel cleanup should be improved

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

RFR 8080225 FileInput/OutputStream/FileChannel cleanup should be improved

roger riggs
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: RFR 8080225 FileInput/OutputStream/FileChannel cleanup should be improved

Peter Levart
Hi Rogger,

On 12/04/2017 02:17 PM, Peter Levart wrote:

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

Ah, scrap that. I forgot that XXXStream.finalize() is now empty, so user
overriding it and calling super does not in fact close the stream. You
have to register AltFinalizer in that case. But now I wonder if the
logic should still be 3-state and do the following:

- if user overrides finalize() - use AltFinalizer to call both: first
finalize() and then close(); else
- if user overrides close() - use AltFinalizer to call close(); else
- use Cleaner

What do you think?

Regards, Peter

Reply | Threaded
Open this post in threaded view
|

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

Peter Levart
This is a nice exercise in compatible migration from finalize() to
Cleaner. The compatibility would be even better with a little tweak to
the VM. Currently VM ignores empty finalize() method(s) and does not
register the instance for finalization if it has empty finalize() (just
return bytecode). There could be a special method-level runtime
annotation, say @IgnoreFinalize that would be used to annotate no-empty
finalize() methods and would have the same effect as empty method on the
VM behavior. XXXStream could then just annotate the finalize() and leave
it as is so that potential overriders of finalize() could call super.

Current approach (with AltFinalizer) is an approximation since it can
only call finalize() and close() in succession. If overridden finalize()
callse super.finalize() in the middle of the method, it may expect the
stream to be already closed for the rest of the method:

@Override
protected finalize() {
     ...
     ... pre-close logic
     ...
     super.finalize();
     ...
     ... post-close logic (may expect stream to be already closed)
     ...
}


...bust since super.close() is empty, the stream is not closed yet and
will be closed by AltFinalizer after this.finalize() returns.

I don't know what impact does such order have on the compatibility.
Probably not big.


Regards, Peter


On 12/04/2017 02:25 PM, Peter Levart wrote:

> Hi Rogger,
>
> On 12/04/2017 02:17 PM, Peter Levart wrote:
>> 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.
>
> Ah, scrap that. I forgot that XXXStream.finalize() is now empty, so
> user overriding it and calling super does not in fact close the
> stream. You have to register AltFinalizer in that case. But now I
> wonder if the logic should still be 3-state and do the following:
>
> - if user overrides finalize() - use AltFinalizer to call both: first
> finalize() and then close(); else
> - if user overrides close() - use AltFinalizer to call close(); else
> - use Cleaner
>
> What do you think?
>
> Regards, Peter
>

Reply | Threaded
Open this post in threaded view
|

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

Peter Levart
In reply to this post by Peter Levart


On 12/04/2017 02:25 PM, Peter Levart wrote:

> Hi Rogger,
>
> On 12/04/2017 02:17 PM, Peter Levart wrote:
>> 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.
>
> Ah, scrap that. I forgot that XXXStream.finalize() is now empty, so
> user overriding it and calling super does not in fact close the
> stream. You have to register AltFinalizer in that case. But now I
> wonder if the logic should still be 3-state and do the following:
>
> - if user overrides finalize() - use AltFinalizer to call both: first
> finalize() and then close(); else
> - if user overrides close() - use AltFinalizer to call close(); else
> - use Cleaner
>
> What do you think?
>
> Regards, Peter
>

I just realized that in the above case when finalize is overridden, it
would be called twice. once by finalization and once by AltFinalizer. So
your logic is as correct as it can be for that case (to just call
close() with AltFinalizer). The only problem is order which is
arbitrary, so it may happen that AltFinalizer calls close() 1st and then
finalization calls overridden finalize() method which might expect the
stream to still be open until it calls super.finalize().

Regards, Peter

Reply | Threaded
Open this post in threaded view
|

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

Peter Levart
Hi Roger,

On 12/04/2017 03:09 PM, Peter Levart wrote:

>
>
> On 12/04/2017 02:25 PM, Peter Levart wrote:
>> Hi Rogger,
>>
>> On 12/04/2017 02:17 PM, Peter Levart wrote:
>>> 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.
>>
>> Ah, scrap that. I forgot that XXXStream.finalize() is now empty, so
>> user overriding it and calling super does not in fact close the
>> stream. You have to register AltFinalizer in that case. But now I
>> wonder if the logic should still be 3-state and do the following:
>>
>> - if user overrides finalize() - use AltFinalizer to call both: first
>> finalize() and then close(); else
>> - if user overrides close() - use AltFinalizer to call close(); else
>> - use Cleaner
>>
>> What do you think?
>>
>> Regards, Peter
>>
>
> I just realized that in the above case when finalize is overridden, it
> would be called twice. once by finalization and once by AltFinalizer.
> So your logic is as correct as it can be for that case (to just call
> close() with AltFinalizer). The only problem is order which is
> arbitrary, so it may happen that AltFinalizer calls close() 1st and
> then finalization calls overridden finalize() method which might
> expect the stream to still be open until it calls super.finalize().
>
> Regards, Peter
>

Final refinement... (hopefully!)

If user overrides just finalize() and does not override close(), then it
might be best to employ Cleaner to close the stream. Cleaner is Phantom
based and will get fired after finalization invokes overridden
finalize(), enabling the finalize() method to still access the stream in
that case. So this is the final logic (I think):

- if user overrides close(), use AltFinalizer to call close(); else
- use Cleaner to close the stream

The above logic in action:

finalize()              close()                        action

not overridden    not overridden           Cleaner closes stream
not overridden    overridden                 AltFinalizer calls close()
overridden          not overridden           finalization calls
finalize() then Cleaner closes stream
overridden          overridden                 finalization calls
finalize() and AltFinalizer calls close() (in arbitrary order)


Regards, Peter

Reply | Threaded
Open this post in threaded view
|

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

roger riggs
Hi Peter,

Thanks for reviewing.

This is a transition step to removing the finalize method completely
while giving subclasses
notice to upgrade their cleanup activities and yet gain the performance
benefits sooner.
Later, finalize() and related compatibility mechanisms will be removed.
Simpler code, even if sometimes less than optimal is preferred to
maintain compatibility in the interim.

...

On 12/4/2017 9:25 AM, Peter Levart wrote:

> Hi Roger,
>
> On 12/04/2017 03:09 PM, Peter Levart wrote:
>>
>>
>> On 12/04/2017 02:25 PM, Peter Levart wrote:
>>> Hi Rogger,
>>>
>>> On 12/04/2017 02:17 PM, Peter Levart wrote:
>>>> 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.
>>>
>>> Ah, scrap that. I forgot that XXXStream.finalize() is now empty, so
>>> user overriding it and calling super does not in fact close the
>>> stream. You have to register AltFinalizer in that case. But now I
>>> wonder if the logic should still be 3-state and do the following:
>>>
>>> - if user overrides finalize() - use AltFinalizer to call both:
>>> first finalize() and then close(); else
>>> - if user overrides close() - use AltFinalizer to call close(); else
>>> - use Cleaner
>>>
>>> What do you think?
>>>
>>> Regards, Peter
>>>
>>
>> I just realized that in the above case when finalize is overridden,
>> it would be called twice. once by finalization and once by
>> AltFinalizer. So your logic is as correct as it can be for that case
>> (to just call close() with AltFinalizer). The only problem is order
>> which is arbitrary, so it may happen that AltFinalizer calls close()
>> 1st and then finalization calls overridden finalize() method which
>> might expect the stream to still be open until it calls
>> super.finalize().
>>
>> Regards, Peter
>>
>
> Final refinement... (hopefully!)
>
> If user overrides just finalize() and does not override close(), then
> it might be best to employ Cleaner to close the stream. Cleaner is
> Phantom based and will get fired after finalization invokes overridden
> finalize(), enabling the finalize() method to still access the stream
> in that case. So this is the final logic (I think):
>
> - if user overrides close(), use AltFinalizer to call close(); else
> - use Cleaner to close the stream
>
> The above logic in action:
>
> finalize()              close()                        action
>
> not overridden    not overridden           Cleaner closes stream
> not overridden    overridden                 AltFinalizer calls close()
> overridden          not overridden           finalization calls
> finalize() then Cleaner closes stream
> overridden          overridden                 finalization calls
> finalize() and AltFinalizer calls close() (in arbitrary order)

Right, if close() is not overridden the behavior of close() is known and
the Cleaner can be used.

The contents of an overridden finalize() method are unknowable, it may
or may not call close() itself
and may or may not call super.finalize().  If  close() is called, then
the Cleaner will be removed; otherwise it will release the resources.
The close method should be idempotent, calling it twice should not be a
problem.

This does simplify the logic.

Thanks, Roger


>
>
> Regards, Peter
>

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by roger riggs


On 12/1/17 6:38 PM, 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.

This revised approach sounds reasonable that minimizes the compatibility
risk in JDK 10 while giving an advanced notice to existing applications
to prepare for the removal of FIS/FOS finalizer method and existing
subclasses that override the close method should make appropriate change
to prepare the incompatibility risk when the FIS/FOS finalizer is
removed in a future release.

Thanks for doing this.
>
> Webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
>

Looks good overall.   I agree with Peter's suggestion that subclasses
overriding the close method will use AltFinalizer and subclasses
overriding the finalizer but not close method will get invoked anyway
and it can employ the cleaner to close the stream in that case.  I see
the webrev has been updated to reflect that.

Minor comments:

Nit: I wonder if {@link #close} should be used instead of {@linkplain
#close} in the javadoc (as I read that referring to the method rather
than the action).   Just to mention it.

The javadoc of the close method:

343 * @implNote
344 * Overriding {@linkplain #close} to perform cleanup actions is reliable
345 * only when called directly or when called by try-with-resources.

I think this can be @apiNote, like the @apiNote added in the finalize
method.  Maybe you want to promote this to @apiNote when the finalize
method is removed.

test/jdk/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java
Nit: @modules would be good to keep it sorted. Formatting nit: line
84-106: it reads to me that the indentation is 5-space

thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

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

roger riggs
Hi Mandy,

Updated the webrev in place:
http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/


On 12/5/2017 2:27 PM, mandy chung wrote:

>
>
> On 12/1/17 6:38 PM, 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.
>
> This revised approach sounds reasonable that minimizes the
> compatibility risk in JDK 10 while giving an advanced notice to
> existing applications to prepare for the removal of FIS/FOS finalizer
> method and existing subclasses that override the close method should
> make appropriate change to prepare the incompatibility risk when the
> FIS/FOS finalizer is removed in a future release.
>
> Thanks for doing this.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
>>
>
> Looks good overall.   I agree with Peter's suggestion that subclasses
> overriding the close method will use AltFinalizer and subclasses
> overriding the finalizer but not close method will get invoked anyway
> and it can employ the cleaner to close the stream in that case.  I see
> the webrev has been updated to reflect that.
Thanks for confirming
>
> Minor comments:
>
> Nit: I wonder if {@link #close} should be used instead of {@linkplain
> #close} in the javadoc (as I read that referring to the method rather
> than the action).   Just to mention it.
>
done
> The javadoc of the close method:
> 343 * @implNote
> 344 * Overriding {@linkplain #close} to perform cleanup actions is
> reliable
> 345 * only when called directly or when called by try-with-resources.
> I think this can be @apiNote, like the @apiNote added in the finalize
> method.  Maybe you want to promote this to @apiNote when the finalize
> method is removed.
>
Yes, it is more like an apiNote
> test/jdk/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java
> Nit: @modules would be good to keep it sorted. Formatting nit: line
> 84-106: it reads to me that the indentation is 5-space
fixed

Thanks, Roger


Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung


On 12/5/17 12:01 PM, Roger Riggs wrote:
> Hi Mandy,
>
> Updated the webrev in place:
> http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
>

Thanks.  Looks good.

Mandy

Reply | Threaded
Open this post in threaded view
|

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

Brian Burkhalter-2
On Dec 5, 2017, at 1:23 PM, mandy chung <[hidden email]> wrote:

> On 12/5/17 12:01 PM, Roger Riggs wrote:
>> Hi Mandy,
>>
>> Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
>>
>
> Thanks.  Looks good.

+1

Brian