Hi Peter,
I filed a new issue for the cleanup:
https://bugs.openjdk.java.net/browse/JDK-8196716
On 2/2/2018 1:16 PM, Peter Levart wrote:
Hi Roger,
Nice separation of concerns (io vs. net).
Is JavaIOFileDescriptorAccess.registerCleanup(FileDescriptor)
currently used at all?
I have not gotten around to looking at the channels implementations of
freeing files/sockets.
and it should be able to use the normal cleanup of raw fd/handles.
Windows networking is asymmetric necessitating the alternate cleanup for
sockets.
Although not necessary for this patch, but to make code more
symmetric, FileDecriptor.FDCleaner could also be extracted into a
package-private top class and equipped with similar static
(un)registration methods as SocketCleanable (possibly also renamed to
FileCleanable and made hosting the native method). You could then
remove the overload of FileDescriptor.registerCleanup() and
corresponding JavaIOFileDescriptorAccess method and make calling code
use FileCleanable.(un)register methods instead. You could then add
checking for non-nullness of PhantomCleanable in
FileDescriptor.registerCleanup(PhantomCleanable) or make the null
value behave as unregisterCleanup() so that you only need one method.
Currently if you mistakenly pass null to this method from socket code,
FDCleanup gets registered.
As FileDescriptor is a public class, you could also make FileCleanable
and SocketCleanable extend PhantomCleanable<FileDescriptor> and make
the method have the following signature
FileDescriptor.registerCleanup(PhantomCleanable<FileDescriptor>).
The code is good as is, but it's a little harder to follow since it is
different for two similar use cases (net vs. io).
What do you think?
I'll closer look next week.
Thanks, Roger
Regards, Peter
On 02/02/18 18:07, Roger Riggs wrote:
Hi Chris,
Updated in place.
http://cr.openjdk.java.net/~rriggs/webrev-net-cleanup-8195059/
Thanks, Roger
On 2/2/2018 11:30 AM, Chris Hegarty wrote:
Roger,
On 01/02/18 21:29, Roger Riggs wrote:
Hi Chris,
Thanks for the review and suggestion.
Webrev updated:
http://cr.openjdk.java.net/~rriggs/webrev-net-cleanup-8195059/
This looks good to me, just a few small comments:
1) windows SocketImpl.c in the comments:
java_net_AbstractPlainSocketImpl ->
java_net_SocketCleanable_cleanupClose0
2) SocketCleanable.java needs a copyright header
-Chris.
* Refactored SocketCleanup into a java.net package private
SocketCleanable
* Moved register and unregister into static methods in
SocketCleanable
* Simplified the test by retaining the checking for GC reclaimed
objects but
removed checking fd counts, since they were unreliable in testing
and not consistent across OS's
Thanks, Roger
On 2/1/2018 10:11 AM, Chris Hegarty wrote:
Hi Roger,
On 31 Jan 2018, at 15:52, Roger Riggs<roger.ri...@oracle.com> wrote:
addingnet-...@openjdk.java.net
On 1/30/2018 5:08 PM, Roger Riggs wrote:
Please review changes to replace finalizers in socket, datagram,
and multicast networking
with Cleaner based release of the raw file descriptors. Each
FileDescriptor is registered
for cleanup after the raw fd (or handle) is assigned. Normal
calls to close unregister the
cleaner before using the current logic to close the raw
fd/handle. Windows networking
uses fd's with the Windows socket_ API requiring a special cased
Cleanable.
The tests check that the implementation objects including
FileDescriptors are reclaimed
and for Linux that the raw fd counts are reduced as expected.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-net-cleanup-8195059/
I think this is good. One small comment; could SocketCleanup be a
top-level package-private class, since it is shared by the three
different socket types.
I didn’t look too hard at the tests, other than to note that they
seem
to verify expected behaviour, which is good.
-Chris.
