RFR 8195059: Update java.net Socket and DatagramSocket implementations to use Cleaner

[Roger Riggs]

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.Riggs at oracle.com> wrote:
>>>>>>
>>>>>> Addingnet-dev at 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.
>>>>
>>
>