Hi Alan,
Thanks for your further comments.
I've refactored DatagramSocket as you suggested, and returned
DatagramSocketAdaptor to the correct formatting. I also removed the
incorrect @bug tag from SetGetSendBufferSize and /othervm from the other
test updates.I've included these changes in the updated webrev below.
WRT the PDSI issue, I've created a bug to track this and have assigned
it to myself. You can view the issue here:
https://bugs.openjdk.java.net/browse/JDK-8242885
webrev: http://cr.openjdk.java.net/~pconcannon/8241072/webrevs/webrev.04/
Kind regards,
Patrick
On 12/04/2020 16:51, Alan Bateman wrote:
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.
