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.
