RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Patrick Concannon]

Hi Alan,

Thanks for your feedback.

I've incorporated your comments into the revised webrev below.

http://cr.openjdk.java.net/~pconcannon/8241072/webrevs/webrev.01/


With regards to the UnreferencedXXX tests, I can take a look at these 
separately.

-Patrick

On 23/03/2020 18:59, Alan Bateman wrote:
> On 23/03/2020 16:37, Patrick Concannon wrote:
>> This is a request for review for the implementation changes for JEP 
>> 373: 'Reimplement Legacy DatagramSocket API' [1]. A CSR has also been 
>> created [2].
>>
>> The implementation of JEP 373 can be viewed on the JDK-8230211-branch 
>> in the sandbox [3].
>>
>> A link to the webrev with the cumulated changes is available below [4].
>>
>> The sources changes are limited to five files. The webrev also adds 
>> additional tests while a selection of existing tests have been 
>> updated to run with both the old and new implementation. Please refer 
>> to the JEP [1] for more information.
> This is a good work and easy to review.
>
> As you know, I would have preferred to see the creation of the 
> delegate separated from the configuration (like we do with impls) but 
> I think you and Daniel want to keep the version you have, okay. I'm 
> also not too keen on another temporary solution in advance of any 
> discussion on JDK-8237352 but I guess what you have is okay as it's 
> benign.
>
> Small comments:
>
> DatagramSocket
> - the comments at L117-120, L123-124, and L143-L147 need a tidy up, 
> probably most of them can be replaced with simple one-liners.
> - the use of type.cast at L119 and L1125 looks odd. It might be 
> simpler to just replace L1156 with "return (T) delegate". That would 
> allow you to assign delegate (no need for "adaptor" or oddly named 
> "plainDelegate").
> - I assume the new static final fields can be private too.
> - usePlainDatagramSocketImpl is inconsistent with the equivalent in 
> SocketImpl for the garbage value case. We should try to get those 
> consistent at some point.
> - createDelegate's method description has inconsistent formatting.
> - L1145 should be "initialized = true".
>
> MulticastSocket and NetMulticastSocket look okay. There are probably 
> better names for "NetMulticastSocket" but it's not public so okay. 
> Happy to see NetMulticastSocket.options is an instance field, seems 
> like that bug was lurking for a long time.
>
> DatagramSocketAdaptor - if we are keeping this change then I'd prefer 
> if the comment was cleaned up to be consistent with the other support 
> classes. A one-liner is okay. The "Temporary solution" comment can go 
> away. Also would be good to use 4-space indentation to be consistent 
> with the other support classes.
>
> The changes to run the tests twice is good. The UnreferencedXXX tests 
> are a bit messy and I'm concerned that the changes to these tests make 
> them too hard to maintain. I think we need to find better ways to test 
> that unreferenced sockets are closed so that these tests can be 
> replaced with tests that easy to maintain.
>
> SendBufCheck is puzzling to see in this webrev as it's not part of the 
> JEP. An equivalent of JDK-8239355 that only tests the new 
> implementation (not the old because it's not quite right) would be 
> okay. Alternatively re-visit it as part of JDK-8240901 to test that 
> large datagrams can be sent on the network.
>
> I think that is all I have.
>
> -Alan