JDK 10 RFR of 8147615: (fc) FileChannelImpl has no finalizer

[Alan Bateman]

On 01/09/2017 15:56, Brian Burkhalter wrote:
> Please review at your convenience:
>
> https://bugs.openjdk.java.net/browse/JDK-8147615
> http://cr.openjdk.java.net/~bpb/8147615/webrev.00/
>
> There is a question at line 106 of FileChannelImpl.java which needs to be resolved.
>
> Another question is whether a synchronized block should be used in or around invalidateAndClose() in addition to relying on the pseudo-synchronization of checking the FileDescriptor validity. It is necessary to invalidate the FileDescriptor in any case as otherwise “Bad file descriptor” exceptions occur. This is likely due to the cleaning action attempting to close a native file descriptor which was already closed when the channel was closed.
>
> That this actually works was verified by instrumenting FileChannelImpl$Closer#run() with a print statement which was since removed. The issue has however been labelled “noreg-hard” as there appears to be no way to verify the fix without such instrumentation or via existing public APIs.
>
Yes, this is hard to test as you will likely run out of file descriptors 
before the cleaner runs. One thing you could do is use the management 
API and check if the OperatingSystemMXBean is a 
UnixOperatingSystemMXBean as that will give you an easy way to get the 
maximum and open file descriptors.

As regards the patch then I assume you can skip creating the Cleaner 
when there is a FileInputStream or FileOutputStream parent (as they have 
finalizers to close the file, also a reference to the channel). If you 
do that then it reduces the scenarios to think about to just the 
FileChannels that are created directly and then unreferenced without 
calling close.

As regards setting the fd to -1 then this is needed to avoid the cleaner 
closing a file descriptor that has been recycled to another file or 
socket or something else. You can synchronize that code. The "what is 
close fails" is difficult. In other areas then failure will terminate 
the VM but I don't think this make sense here close might fail with EIO. 
Minimally EINTR will need special handling as you won't want an 
exception or logging for that case. The simplest might be to just for 
ignore and pick up again if/when logging in core libraries is examined.

-Alan