RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Alan Bateman]

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