RFR: 8245194: Unix domain socket channel implementation [v18]
Alan Bateman
Alan.Bateman at oracle.com
Sun Oct 11 15:48:37 UTC 2020
I went through the latest patch, v18.
I'm still a bit uncomfortable with splitting up SocketChannelImpl and
ServerSocketChannelImpl but okay with go along with this for now if we
can continue explore alternatives. Given that the InetXXX and UnixXXX
sub-classes aren't too big then one thing that we can try is just
special casing the protocol family in the 4-5 methods that are
different. In the mean-time, I think the abstract methods in
SocketChannelImpl and ServerSocketChannelImpl should have a clear method
description on what the implXXX method should. I would prefer the other
methods be changed to final so that it's clear that they cannot be
overridden. implSocket is new in v17 or v18 and I'd prefer if that were
abstract so it can be implemented in UnixXXX to throw UOE.
I think the JFR events need another look. Are UnixSocketReadEvent and
UnixSocketWriteEvent really needed? SocketXXXEvent "address" is just a
String so it can be used for any address type, the port can be left as
0. Part of this comment is wondering if a parallel set of events are
really needed. Part of it that I would prefer if the read/write methods
were final in SocketChannelImpl.
You might want to take a pass over the files in the patch as several of
the new files have copyright dates starting 2012, 2019 and other years,
probably initially copied from other files.
A few specific comments on v18:
src/java.base/share/classes/sun/nio/ch/UnixDomainSocketChannelImpl.java
- supportedOptions doesn't need to wrap the unmodifable collection with
Collections.unmodifiableSet
- I think implBind should be re-worked so the local != null case is not
handled in the loop body. That will help future maintainers.
src/java.base/windows/classes/sun/nio/ch/UnixDomainSocketsUtil.java
- I think we should look at adding JAVA_IO_TMPDIR to StaticProperty.
src/java.base/unix/native/libnio/ch/UnixDomainSockets.c
- are the AIX specific includes really needed?
- can unixSocketAddressToSockaddr use if-then-else to avoid the goto.
- bind0 checks if the path is NULL, is this a left over?
- the declaration of accept0 is mis-aligned, might be left over from
refactoring
src/java.base/unix/native/libnio/ch/InheritedChannel.c
- might be clearer if matchFamily is renamed to toInetFamily
src/java.base/unix/classes/sun/nio/ch/InheritedChannel.java
- peerAddress0 is replaced by peerAddressInet and peerAddressUnix0 which
looks a bit consistent, maybe rename those to inetPeerAddress0 and
unixPeerAddress0 ?
src/java.base/windows/classes/sun/nio/ch/PipeImpl.java
- we've gone through a few iterations on this and only a few minor comments
- source/sink can be final and I assume no need to change their type
from SourceChannel/SinkChannel
- no need to initialize noUnixDomainSockets to false
- can the comments at L178-179 be turned into a method description.
Can the tests for java.net.UnixDomainSocketAddress be moved to
jdk/test/java/net/UnixSocketAddress?
Could test/jdk/java/nio/file/spi/TestProvider.java be used as
alternative provider in the test to avoid DummyPath?
test/jdk/java/nio/channels/etc/ProtocolFamilies.java has been updated to
add "expectedFamily", is that used?
test/jdk/java/nio/channels/unixdomain/WindowsDriver.java and
NonWindowsDriver.java
- are these needed now? Both of them run the same 5 tests and not clear
why these tests don't have their own @test tags.
test/jdk/java/nio/channels/unixdomain/Security.java
- is there any reason why this test can't run with run
main/othervm/java.security.policy=java.policy1 -Djava.security.manager ?
That's all I have from this pass over the changes.
-Alan
More information about the nio-dev
mailing list