RFR 8080225 FileInput/OutputStream/FileChannel cleanup should be improved
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
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
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
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
On Dec 5, 2017, at 1:23 PM, mandy chung <mandy.chung@oracle.com> 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
participants (3)
-
Brian Burkhalter
-
mandy chung
-
Roger Riggs