Hi Alan, Here is the new modification of the patch. The link is available here: http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.10/
In this version, we make supportedOptions() in AbstractPlainSocketImpl and AbstractPlainDatagramSocketImpl return an immutable set of the socket options. In addition, we corrected couple formatting issues. Please let us know your feedback and comments. Thanks, Lucy -----Original Message----- From: Alan Bateman [mailto:alan.bate...@oracle.com] Sent: Friday, January 29, 2016 6:38 AM To: Lu, Yingqi <yingqi...@intel.com>; Volker Simonis <volker.simo...@gmail.com>; Michael McMahon <michael.x.mcma...@oracle.com> Cc: net-dev@openjdk.java.net; Viswanathan, Sandhya <sandhya.viswanat...@intel.com>; Kharbas, Kishor <kishor.khar...@intel.com>; Aundhe, Shirish <shirish.aun...@intel.com>; Kaczmarek, Eric <eric.kaczma...@intel.com> Subject: Re: Patch for adding SO_REUSEPORT socket option 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.
