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