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
