RFR 8080225: FileInputStream cleanup should be improved
Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor. The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3]. The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable. The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams. Some tests were modified to not rely on finalization; new tests are provided. Comments are appreciated on both the CSR [3] and the implementation [1]. [1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/ [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225 [3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325 Thanks, Roger
Hi Roger, Looks good overall; total nitpicks here: FileInputStream (similar story in FileOutputStream) 48-49: “explicitly" is used twice 53: could probably drop “explicitly” here altogether. The “Shd” in a couple of test names is kind of annoying; perhaps s/Shd/Should/ ? Copyright dates are not update in a few places. Thanks, Brian On Sep 29, 2017, at 10:17 AM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
Comments are appreciated on […] the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
Hello, if UnreferencedRAFClosesFd.java is supposed to test the behavior of unreachable ˋrafˋ I wonder how it works as it hold on raf with a reachabilityFence at the end of the main method? Maybe that asks for an explanative comment? Gruss Bernd -- http://bernd.eckenfels.net ________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Roger Riggs <Roger.Riggs@Oracle.com> Sent: Friday, September 29, 2017 7:17:34 PM To: Core-Libs-Dev Subject: RFR 8080225: FileInputStream cleanup should be improved Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor. The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3]. The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable. The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams. Some tests were modified to not rely on finalization; new tests are provided. Comments are appreciated on both the CSR [3] and the implementation [1]. [1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/ [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225 [3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325 Thanks, Roger
Hi Bernd, The reachabilityFence only ensures that the local variable 'ref' is not prematurely deemed to be out of scope by the hotspot compiler. The value of 'raf' is set to null before the gc is triggered (usually) on the first iteration of the loop. I'll add a comment. Regards, Roger On 9/30/2017 12:34 AM, Bernd Eckenfels wrote:
Hello,
if UnreferencedRAFClosesFd.java is supposed to test the behavior of unreachable ˋrafˋ I wonder how it works as it hold on raf with a reachabilityFence at the end of the main method? Maybe that asks for an explanative comment?
Gruss Bernd -- http://bernd.eckenfels.net ------------------------------------------------------------------------ *From:* core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Roger Riggs <Roger.Riggs@Oracle.com> *Sent:* Friday, September 29, 2017 7:17:34 PM *To:* Core-Libs-Dev *Subject:* RFR 8080225: FileInputStream cleanup should be improved Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor.
The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3].
The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable.
The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams.
Some tests were modified to not rely on finalization; new tests are provided.
Comments are appreciated on both the CSR [3] and the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/ <http://cr.openjdk.java.net/%7Erriggs/webrev-fis-cleanup-8080225/>
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
[3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325
Thanks, Roger
Hi Roger, This looks good overall. 53 * unreachable should explicitly override {@link #finalize} and call {@code close}. Since finalize is deprecated, I would not recommend to have the subclass adding the finalize method. I suggest to take that phrase out (I gave a similar comment to JDK-8185582 [1]). Users should use try-with-resource or register the instances in a cleaner. As for the tests, FinalizeShdCallClose.java implements finalize. I think it'd be good to convert them to try-with-resource. I'm not close to java.io implementation. Would registerCleanup be called more than once? line 220 in the registerCleanup will create a new cleanup if it's invoked the second time - is it intentional? 216 synchronized void registerCleanup() { 217 if (cleanup != null) { 218 cleanup.clear(); 219 } 220 cleanup = FDCleanup.create(this); 221 } thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049362.h... On 9/29/17 10:17 AM, Roger Riggs wrote:
Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor.
The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3].
The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable.
The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams.
Some tests were modified to not rely on finalization; new tests are provided.
Comments are appreciated on both the CSR [3] and the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
[3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325
Thanks, Roger
Hi Mandy, Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/ On 10/3/2017 7:17 PM, mandy chung wrote:
Hi Roger,
This looks good overall.
53 * unreachable should explicitly override {@link #finalize} and call {@code close}.
Since finalize is deprecated, I would not recommend to have the subclass adding the finalize method. I suggest to take that phrase out (I gave a similar comment to JDK-8185582 [1]). Users should use try-with-resource or register the instances in a cleaner. ok, but neither t-w-r or cleaner can completely replace the behavior of finalize. In some cases, significant refactoring of the application class may be needed.
As for the tests, FinalizeShdCallClose.java implements finalize. I think it'd be good to convert them to try-with-resource. The new tests Unreferenced*ClosesFd incorporate the original FinalizeShdCallClose tests cases except the case where the subclass does override finalize to call closed; behavior that is deprecated but still supported. I refactored the Unreferenced*ClosesFd tests to make the FinalizeShdCallClose tests unnecessary and will remove them. [If the CSR requires greater behavioral compatibility, the tests may be needed again].
I'm not close to java.io implementation. Would registerCleanup be called more than once? line 220 in the registerCleanup will create a new cleanup if it's invoked the second time - is it intentional? 216 synchronized void registerCleanup() { 217 if (cleanup != null) { 218 cleanup.clear(); 219 } 220 cleanup = FDCleanup.create(this); 221 }
Re-registering *clears* not cleans the previous cleanup and then creates a new one. If the native fd has changed a new cleanup is needed. There no cases currently where registerCleanup is called twice on the same FileDescriptor. Thanks, Roger
thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049362.h...
On 9/29/17 10:17 AM, Roger Riggs wrote:
Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor.
The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3].
The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable.
The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams.
Some tests were modified to not rely on finalization; new tests are provided.
Comments are appreciated on both the CSR [3] and the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
[3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325
Thanks, Roger
On 04/10/2017 15:35, Roger Riggs wrote:
Hi Mandy,
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/ I skimmed the latest webrev.
The @apiNote in FIS is copied from FOS so it needs s/FileOutputStream/FileInputStream/. I assume in FIS that we should skip registerCleanup when the fd is FileDescriptor.in, ditto in FOS and RAF where we shouldn't register for cleanup when the fd is one of the standard streams. Alternatively handle this in FileDescriptor. I'm curious why registerCleaner invoke clears and create a new cleanup. There are cases where we create a FileDescriptor and set the fd or handle lazily but these won't be registered until set. Otherwise I think this looks quite good. -Alan
Hi Alan, On 10/12/2017 8:35 AM, Alan Bateman wrote:
On 04/10/2017 15:35, Roger Riggs wrote:
Hi Mandy,
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/ I skimmed the latest webrev.
The @apiNote in FIS is copied from FOS so it needs s/FileOutputStream/FileInputStream/. fixed
I assume in FIS that we should skip registerCleanup when the fd is FileDescriptor.in, ditto in FOS and RAF where we shouldn't register for cleanup when the fd is one of the standard streams. Alternatively handle this in FileDescriptor. The FileDescriptors for in, out, err are held in static fields and should never be only phantom reachable. The registration of cleanup handlers only occurs when opening files, not for constructors that accept a FileDescriptor. So there will be no cleanup registrations for fd 0,1,2. (unless I'm missing a case)
I'm curious why registerCleaner invoke clears and create a new cleanup. There are cases where we create a FileDescriptor and set the fd or handle lazily but these won't be registered until set.
Just an abundance of caution, currently there are no calls with set(-1) but if there were the cleaner should be reset to avoid closing a stale fd at some future time. Thanks, Roger
Otherwise I think this looks quite good.
-Alan
On 10/4/17 7:35 AM, Roger Riggs wrote:
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
Looks good. One minor comment: 251 private static native void cleanupClose0(int fd); 75 Java_java_io_FileDescriptor_cleanupClose0(JNIEnv *env, jclass fdClass, jint fd) { 76 if (close(fd) == -1) { 77 JNU_ThrowIOExceptionWithLastError(env, "close failed"); 78 } 79 } The native implementation throws IOException when there is any error. Not directly related to this change: when there is any error in closing FIS/FOS, the exception will be ignored by the cleaner. I wonder if we could enhance the Cleaner's diagnosability to log any exception thrown; of course this is a separate RFE that might have been considered. Mandy
Hi Mandy, True in advertising, the FileDescriptor.close() method should be declared to throw IOException. Cleanup via the Cleaner.clean() method can't throw a checked exception but the IOException can be wrapped with UncheckedIOException that can be caught by Cleaner.clean() callers. They can unwrap the exception as needed. When invoked from the Cleaner thread, all exceptions are ignored. Perhaps that is a better place to log unexpected exceptions. Roger On 10/12/2017 2:00 PM, mandy chung wrote:
On 10/4/17 7:35 AM, Roger Riggs wrote:
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
Looks good. One minor comment: 251 private static native void cleanupClose0(int fd); 75 Java_java_io_FileDescriptor_cleanupClose0(JNIEnv *env, jclass fdClass, jint fd) { 76 if (close(fd) == -1) { 77 JNU_ThrowIOExceptionWithLastError(env, "close failed"); 78 } 79 } The native implementation throws IOException when there is any error.
Not directly related to this change: when there is any error in closing FIS/FOS, the exception will be ignored by the cleaner. I wonder if we could enhance the Cleaner's diagnosability to log any exception thrown; of course this is a separate RFE that might have been considered.
Mandy
Hi Roger, I know I'm late for the comments on this one, but anyway... I'm looking at JNI part of FileDescriptor. There are now two native "close" methods for each FileDescriptor variant (unix/windows). One is instance method (close0) and the other is static (cleanupClose0), which takes an int fd / long handle argument. close0 delegates to C method "fileDescriptorClose", which could be implemented entirely in Java with a little help from static cleanupClose0, don't you think? This could be a follow-up cleanup effort. Regards, Peter On 10/04/17 16:35, Roger Riggs wrote:
Hi Mandy,
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
On 10/3/2017 7:17 PM, mandy chung wrote:
Hi Roger,
This looks good overall.
53 * unreachable should explicitly override {@link #finalize} and call {@code close}.
Since finalize is deprecated, I would not recommend to have the subclass adding the finalize method. I suggest to take that phrase out (I gave a similar comment to JDK-8185582 [1]). Users should use try-with-resource or register the instances in a cleaner. ok, but neither t-w-r or cleaner can completely replace the behavior of finalize. In some cases, significant refactoring of the application class may be needed.
As for the tests, FinalizeShdCallClose.java implements finalize. I think it'd be good to convert them to try-with-resource. The new tests Unreferenced*ClosesFd incorporate the original FinalizeShdCallClose tests cases except the case where the subclass does override finalize to call closed; behavior that is deprecated but still supported. I refactored the Unreferenced*ClosesFd tests to make the FinalizeShdCallClose tests unnecessary and will remove them. [If the CSR requires greater behavioral compatibility, the tests may be needed again].
I'm not close to java.io implementation. Would registerCleanup be called more than once? line 220 in the registerCleanup will create a new cleanup if it's invoked the second time - is it intentional? 216 synchronized void registerCleanup() { 217 if (cleanup != null) { 218 cleanup.clear(); 219 } 220 cleanup = FDCleanup.create(this); 221 }
Re-registering *clears* not cleans the previous cleanup and then creates a new one. If the native fd has changed a new cleanup is needed. There no cases currently where registerCleanup is called twice on the same FileDescriptor.
Thanks, Roger
thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049362.h...
On 9/29/17 10:17 AM, Roger Riggs wrote:
Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor.
The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3].
The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable.
The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams.
Some tests were modified to not rely on finalization; new tests are provided.
Comments are appreciated on both the CSR [3] and the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
[3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325
Thanks, Roger
On 10/14/17 10:51, Peter Levart wrote:
Hi Roger,
I know I'm late for the comments on this one, but anyway...
I'm looking at JNI part of FileDescriptor. There are now two native "close" methods for each FileDescriptor variant (unix/windows). One is instance method (close0) and the other is static (cleanupClose0), which takes an int fd / long handle argument. close0 delegates to C method "fileDescriptorClose", which could be implemented entirely in Java with a little help from static cleanupClose0, don't you think?
Well, there is this complication in UNIX variant about closing 0,1 or 2 descriptors. Perhaps the static cleanupClose0 method could encapsulate this logic and have the following signature: private static native int cleanupClose0(int fd); The all-Java close0() would then use the returned value and replace it in FileDescriptor.fd. What do you think? Regards, Peter
This could be a follow-up cleanup effort.
Regards, Peter
On 10/04/17 16:35, Roger Riggs wrote:
Hi Mandy,
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
On 10/3/2017 7:17 PM, mandy chung wrote:
Hi Roger,
This looks good overall.
53 * unreachable should explicitly override {@link #finalize} and call {@code close}.
Since finalize is deprecated, I would not recommend to have the subclass adding the finalize method. I suggest to take that phrase out (I gave a similar comment to JDK-8185582 [1]). Users should use try-with-resource or register the instances in a cleaner. ok, but neither t-w-r or cleaner can completely replace the behavior of finalize. In some cases, significant refactoring of the application class may be needed.
As for the tests, FinalizeShdCallClose.java implements finalize. I think it'd be good to convert them to try-with-resource. The new tests Unreferenced*ClosesFd incorporate the original FinalizeShdCallClose tests cases except the case where the subclass does override finalize to call closed; behavior that is deprecated but still supported. I refactored the Unreferenced*ClosesFd tests to make the FinalizeShdCallClose tests unnecessary and will remove them. [If the CSR requires greater behavioral compatibility, the tests may be needed again].
I'm not close to java.io implementation. Would registerCleanup be called more than once? line 220 in the registerCleanup will create a new cleanup if it's invoked the second time - is it intentional? 216 synchronized void registerCleanup() { 217 if (cleanup != null) { 218 cleanup.clear(); 219 } 220 cleanup = FDCleanup.create(this); 221 }
Re-registering *clears* not cleans the previous cleanup and then creates a new one. If the native fd has changed a new cleanup is needed. There no cases currently where registerCleanup is called twice on the same FileDescriptor.
Thanks, Roger
thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049362.h...
On 9/29/17 10:17 AM, Roger Riggs wrote:
Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor.
The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3].
The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable.
The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams.
Some tests were modified to not rely on finalization; new tests are provided.
Comments are appreciated on both the CSR [3] and the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
[3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325
Thanks, Roger
Hi Peter, On 10/14/2017 5:00 AM, Peter Levart wrote:
On 10/14/17 10:51, Peter Levart wrote:
Hi Roger,
I know I'm late for the comments on this one, but anyway...
I'm looking at JNI part of FileDescriptor. There are now two native "close" methods for each FileDescriptor variant (unix/windows). One is instance method (close0) and the other is static (cleanupClose0), which takes an int fd / long handle argument. close0 delegates to C method "fileDescriptorClose", which could be implemented entirely in Java with a little help from static cleanupClose0, don't you think?
Well, there is this complication in UNIX variant about closing 0,1 or 2 descriptors. Perhaps the static cleanupClose0 method could encapsulate this logic and have the following signature:
private static native int cleanupClose0(int fd);
The all-Java close0() would then use the returned value and replace it in FileDescriptor.fd.
What do you think?
The native handling of the file descriptor and setting the field to -1 in native code was, I think, trying to avoid races in the closing of the fd. The functions in io_util_md.c are also used for sockets and datagrams in addition to files. It has always been sensitive code. The new CleanupClose0 was intended only for the case where a FD was unreferenced. The System.in/out/err FileDescriptors should never become unreferenced so the redirection to /dev/null is not needed in that case. I filed a cleanup bug: https://bugs.openjdk.java.net/browse/JDK-8189330 to take another look later. Thanks, Roger
Regards, Peter
This could be a follow-up cleanup effort.
Regards, Peter
On 10/04/17 16:35, Roger Riggs wrote:
Hi Mandy,
Updated the webrev in place: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
On 10/3/2017 7:17 PM, mandy chung wrote:
Hi Roger,
This looks good overall.
53 * unreachable should explicitly override {@link #finalize} and call {@code close}.
Since finalize is deprecated, I would not recommend to have the subclass adding the finalize method. I suggest to take that phrase out (I gave a similar comment to JDK-8185582 [1]). Users should use try-with-resource or register the instances in a cleaner. ok, but neither t-w-r or cleaner can completely replace the behavior of finalize. In some cases, significant refactoring of the application class may be needed.
As for the tests, FinalizeShdCallClose.java implements finalize. I think it'd be good to convert them to try-with-resource. The new tests Unreferenced*ClosesFd incorporate the original FinalizeShdCallClose tests cases except the case where the subclass does override finalize to call closed; behavior that is deprecated but still supported. I refactored the Unreferenced*ClosesFd tests to make the FinalizeShdCallClose tests unnecessary and will remove them. [If the CSR requires greater behavioral compatibility, the tests may be needed again].
I'm not close to java.io implementation. Would registerCleanup be called more than once? line 220 in the registerCleanup will create a new cleanup if it's invoked the second time - is it intentional? 216 synchronized void registerCleanup() { 217 if (cleanup != null) { 218 cleanup.clear(); 219 } 220 cleanup = FDCleanup.create(this); 221 }
Re-registering *clears* not cleans the previous cleanup and then creates a new one. If the native fd has changed a new cleanup is needed. There no cases currently where registerCleanup is called twice on the same FileDescriptor.
Thanks, Roger
thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049362.h...
On 9/29/17 10:17 AM, Roger Riggs wrote:
Replacing finalizers in FileInputStream, FileOutputStream, and adding cleanup to RandomAccessFile and NIO File Channels with Cleaner based implementations required changes to FileDescriptor.
The specification of FileInputStream and FileOutputStream is changed to remove the finalizer behavior that required their respective close methods to be called. This change to the behavior is tracked with CSR 8187325 [3].
The FileDescriptor implementations (Unix and Windows) provide a cleanup function that is now used by FIS, FOS, RAF, and async and normal FileChannels. Each requests FileDescriptor to register a cleanup function when the fd or handle is assigned and delegates to FileDescriptor.close. If the respective FileDescriptor.close method is not called, the fd or handle will be closed when the FileDescriptor is determined to be phantom reachable.
The other uses of FileDescriptor are not intended to be changed, for example in sockets and datagrams.
Some tests were modified to not rely on finalization; new tests are provided.
Comments are appreciated on both the CSR [3] and the implementation [1].
[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8080225
[3] CSR: 8187325 FileInputStream/FileOutputStream should use the Cleaner instead of finalize https://bugs.openjdk.java.net/browse/JDK-8187325
Thanks, Roger
participants (6)
-
Alan Bateman
-
Bernd Eckenfels
-
Brian Burkhalter
-
mandy chung
-
Peter Levart
-
Roger Riggs