RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Alan Bateman]

On 09/04/2020 12:26, Patrick Concannon wrote:
>
> Hi,
>
> Alan - I've gone through your comments and refactored the code 
> accordingly.
>
Just a few minor comments on the implementation changes.

DatagramSocket.java
- this version of the webrev has a block comment "A global switch ..." 
that looks like it was added in the wrong place. I assume it can be 
removed or a subset of the text moved into the command for createDelegate
- toSocketException can be private
- the comment on the delegate field could be shortened by dropping 
"`delegate` can be". That is, it would be a clearer for readers to just 
say "any instance ..."
- Another minor nit but the comment on the delegate() method could 
confuse readers. I think you can drop it or just say that it can be 
overridden to return a MulticastSocket.
- The  new // comments added to the asserts at L133-136 aren't useful, I 
suggest just drop them as the asserts are clear as they are.

DatagramSocketAdaptor
- this version has re-formatted existing code in several places. Can you 
undo these changes (I can't tell if they were deliberate or the IDE did 
it). The only changes should be to be super call and the addition of the 
DatagramSockets helper class.

MulticastSocket and NetMulticastSocket look good.

> I spoke with Daniel about the SendBufCheck test, and he wants to keep 
> this in the JEP as it allows to compare the behavior of all the 
> different implementations in one place.
>
I don't think SetGetSendBufferSize should be in the webrev. It has @bug 
8239355 so it's for a different issue.

I also don't think SendBufCheck should be in the webrev (btw: it also 
has @bug 8239355). The underlying issue is that PDSI doesn't allow 
sending IPv6 datagrams on macOS of size 65508-65527 (inclusive) bytes 
without changing the default value of SO_SNDBUF. It's not an interesting 
issue of course but I don't think the tests for the JEP should be adding 
a test to make sure that the old implementation has this bug. Can't the 
PDSI issue be fixed with a different issue? Alternatively adding a test 
that checks that IPv6 datagrams of at least size 65507 can be sent on 
configurations that have IPv6 enabled.

A minor nit for the other test updates is that some of them are changing 
existing tests to run, by default, in /othervm mode. I can't tell if 
that is deliberate or not but we usually try to minimize the use of 
/othervm where possible.

-Alan.