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

[Hans Boehm]

It would be great if this code eventually grew some comments about how we
prevent cleanup code from running while the (low level integer) file
descriptors are in use. AFAICT, this is probably mostly OK here, due to
some combination of descriptor access bottoming out in non-static JNI
methods and synchronized accesses. But the reasoning behind that seems
unpleasantly global. E.g. why is FileDescriptor.getAppend() safe? In this
case we know that the only caller is a constructor, which kind of requires
write to this "afterwards", though that doesn't actually quite seem to
follow 12.6.2 and earlier rules either.

On Fri, Feb 2, 2018 at 10:16 AM, Peter Levart <peter.levart at gmail.com>
wrote:

> Hi Roger,
>
> Nice separation of concerns (io vs. net).
>
> Is JavaIOFileDescriptorAccess.registerCleanup(FileDescriptor) currently
> used at all?
>
> 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?
>
> 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_clean
>>> upClose0
>>>
>>> 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.
>>>>>
>>>>
>>>>
>>
>