8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket

[Daniel Fuchs]

Hi Alan,

Globally this looks good to me. One small nit is that the `oldImpl`
field could now also become final (by applying the same trick you
did with createImpl - that is - have oldImpl() return a boolean
rather than set the field and assign the result in the constructor.

I've imported your patch and am currently running some tests.

best regards,

-- daniel

On 10/01/2020 14:55, Alan Bateman wrote:
> This is a patch to "upgrade" the DatagramChannel socket adaptor to 
> extend MulticastSocket. This is one of the final changes needed to 
> DatagramChannel and its socket adaptor buddy to make it easy to replace 
> the legacy DatagramSocket and MulticastSocket implementation. Daniel has 
> a draft JEP [1] with the all the details on this effort.
> 
> The changes should be straight-forward to review as most of the methods 
> defined by MulticastSocket are easy to implement on DatagramChannel. A 
> few review notes:
> 
> 1. MulticastSocket does not have a protected constructor to create the 
> socket with a custom DatagramSocketImpl. The DatagramSocketImpl 
> mechanism is legacy and not worth adding a constructor now. So the most 
> significant change in the patch is that the socket adaptor is changed to 
> invoke the MulticastSocket(SocketAddress) constructor. This requires a 
> special checked in the MulticastSocket and DatagramSocket constructor 
> that isn't pretty but it has the advantage that the dummy 
> DatagramSocketImpl can go away (it was never used anyway).
> 
> 2. The DatagramSocket.impl field is changed to final. This is nice 
> because it avoids accidents with lazily creating an impl (say where the 
> socket adaptor doesn't override all methods). The final field means 
> there are several opportunities to for cleanup but I don't want to go 
> there in this patch (this is why getImpl continues to declare that it 
> throws SocketException). I also don't want to complicate things for the 
> JEP implementation in the sandbox.
> 
> 3. The MulticastSocket spec in lacking in many areas and doesn't specify 
> the exceptions or behavior for many scenarios. The joinGroup methods 
> don't specify how they behave when already a member of the group. The 
> leaveGroup method doesn't specify the behavior when not a member. 
> Several methods don't specify the exception when invoked with null or 
> other bad input. The overrides in the socket adaptor try to maintain 
> compatibility with the the long standing behavior.
> 
> 4. The DatagramSocket::socket spec is modified to drop the statement 
> that the socket adaptor doesn't have any public methods beyond those 
> defined by DatagramSocket. We need to drop the same statement from 
> SocketChannel and ServerSocketChannel, something for another issue. So I 
> need a CSR and that can also track the observable change that the 
> returned object is a MulticastSocket.
> 
> 5. One issue with the test is that is initially tickled an intermittent 
> bug on macOS. The underlying setsockopt fails intermittently with ENOMEM 
> when attempting to join the group. There have been sightings of this 
> issue going back to JDK 7 [2]. Also sightings with the legacy 
> MulticastSocket implementation. The patch has a temporary workaround to 
> this issue. It's ugly but I can't duplicate it with this change. Daniel 
> has been thinking about doing the same workaround while we figure out if 
> there is a fix for macOS.
> 
> The webrev with the changes is here:
>      http://cr.openjdk.java.net/~alanb/8236925/webrev/
> 
> -Alan
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8235674
> [2] https://mail.openjdk.java.net/pipermail/net-dev/2013-July/006769.html