RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Patrick Concannon]

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.
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20200415/28c0d2e7/attachment.htm>