RFR[8241072]: 'Reimplement Legacy DatagramSocket API'
Alan Bateman
Alan.Bateman at oracle.com
Mon Mar 23 18:59:58 UTC 2020
On 23/03/2020 16:37, Patrick Concannon wrote:
> This is a request for review for the implementation changes for JEP
> 373: 'Reimplement Legacy DatagramSocket API' [1]. A CSR has also been
> created [2].
>
> The implementation of JEP 373 can be viewed on the JDK-8230211-branch
> in the sandbox [3].
>
> A link to the webrev with the cumulated changes is available below [4].
>
> The sources changes are limited to five files. The webrev also adds
> additional tests while a selection of existing tests have been updated
> to run with both the old and new implementation. Please refer to the
> JEP [1] for more information.
This is a good work and easy to review.
As you know, I would have preferred to see the creation of the delegate
separated from the configuration (like we do with impls) but I think you
and Daniel want to keep the version you have, okay. I'm also not too
keen on another temporary solution in advance of any discussion on
JDK-8237352 but I guess what you have is okay as it's benign.
Small comments:
DatagramSocket
- the comments at L117-120, L123-124, and L143-L147 need a tidy up,
probably most of them can be replaced with simple one-liners.
- the use of type.cast at L119 and L1125 looks odd. It might be simpler
to just replace L1156 with "return (T) delegate". That would allow you
to assign delegate (no need for "adaptor" or oddly named "plainDelegate").
- I assume the new static final fields can be private too.
- usePlainDatagramSocketImpl is inconsistent with the equivalent in
SocketImpl for the garbage value case. We should try to get those
consistent at some point.
- createDelegate's method description has inconsistent formatting.
- L1145 should be "initialized = true".
MulticastSocket and NetMulticastSocket look okay. There are probably
better names for "NetMulticastSocket" but it's not public so okay. Happy
to see NetMulticastSocket.options is an instance field, seems like that
bug was lurking for a long time.
DatagramSocketAdaptor - if we are keeping this change then I'd prefer if
the comment was cleaned up to be consistent with the other support
classes. A one-liner is okay. The "Temporary solution" comment can go
away. Also would be good to use 4-space indentation to be consistent
with the other support classes.
The changes to run the tests twice is good. The UnreferencedXXX tests
are a bit messy and I'm concerned that the changes to these tests make
them too hard to maintain. I think we need to find better ways to test
that unreferenced sockets are closed so that these tests can be replaced
with tests that easy to maintain.
SendBufCheck is puzzling to see in this webrev as it's not part of the
JEP. An equivalent of JDK-8239355 that only tests the new implementation
(not the old because it's not quite right) would be okay. Alternatively
re-visit it as part of JDK-8240901 to test that large datagrams can be
sent on the network.
I think that is all I have.
-Alan
More information about the net-dev
mailing list