8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
daniel.fuchs at oracle.com
Tue Jan 14 17:49:58 UTC 2020
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.
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  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 . 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:
>  https://bugs.openjdk.java.net/browse/JDK-8235674
>  https://mail.openjdk.java.net/pipermail/net-dev/2013-July/006769.html
More information about the nio-dev