On 28/01/2016 06:03, Lu, Yingqi wrote:
Hi Alan,
Here is a new version of the patch:
http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.08/
In this version, based on your comment, we have changed following items:
1. Remove the dependency of DatagramSocketImpl and MulticastSocket with
AbstractPlainSocketImpl.
2. Made addReusePortOption() to be both thread safe and package private.
Please let us know your feedback and comments.
I looking the latest patch and it's looking quite good but I think there
is still an issue with the supportedOptions() methods in the java.net
classes. You changes also highlight an existing bug in
SocketImpl::supportedOptions whereby it returns a mutable rather than
immutable set - I'll create a bug for this.
The issue is that the patch changes SocketImpl.socketOptions and
SocketImpl.serverSocketOptions and so these sets of options will be
incorrect if an alternative SocketImpl is created.
For now then I think the simplest is to change
AbstractPlainSocketImpl.supportedOptions() to:
@Override
protected Set<SocketOption<?>> supportedOptions() {
Set<SocketOption<?>> options = super.supportedOptions();
if (isReusePortAvailable()) {
options = new HashSet<>(options);
options.add(StandardSocketOptions.SO_REUSEPORT);
return Collections.unmodifiableSet(options);
} else {
return options;
}
}
That will ensure that the caller get an immutable set of options that
include SO_REUSEPORT when it supported. Clearly it would be better to
cache this but we can sort that later once you are okay with not
changing the set at the level of SocketImpl.
Otherwise just a few minor comments:
- in src/java.base/windows/native/libnet/net_util_md.h then it might be
better to put the #define SO_REUSEPORT near the top or at least before
the function prototypes.
- test/java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java
- I assume this change is not needed.
- test/java/nio/channels/DatagramChannel/SocketOptionTests.java - the
Arrays.asList could be formatted better to keep the line length consistent.
That's all I have, I think we are close to the final line on this one.
-Alan.
