Re: Patch for adding SO_REUSEPORT socket option

Wed, 17 Feb 2016 08:59:12 -0800 

One comment  on the supportability side from me :

java/net/AbstractPlainDatagramSocketImpl.java

> +         case SO_REUSEPORT:
> +             if (o == null || !(o instanceof Boolean)) {
> + throw new SocketException("bad argument for SO_REUSEPORT"); 
> +             }
> + if (!supportedOptions().contains(StandardSocketOptions.SO_REUSEPORT)) { > + throw new UnsupportedOperationException("unsupported option");
Can we improve the message ? e.g "SO_REUSEPORT is an unsupported option" - line occurs twice.
Ditto for java/net/AbstractPlainSocketImpl.java and java/net/PlainDatagramSocketImpl.java and java/net/PlainSocketImpl.java (windows & unix variants)
java/net/DualStackPlainDatagramSocketImpl.java, java/net/DualStackPlainSocketImpl.java, java/net/PlainSocketImpl.java, java/net/TwoStacksPlainDatagramSocketImpl.java and java/net/TwoStacksPlainSocketImpl.java also need tweaking on that message. 

Thanks,
Sean.

On 17/02/16 11:43, Chris Hegarty wrote:
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Lu, Yingqi 
Sent: Thursday, February 11, 2016 9:47 AM
To: Alan Bateman <alan.bate...@oracle.com>; Volker Simonis <volker.simo...@gmail.com>; Michael McMahon <michael.x.mcma...@oracle.com> Cc: Kaczmarek, Eric <eric.kaczma...@intel.com>; Viswanathan, Sandhya <sandhya.viswanat...@intel.com>; Kharbas, Kishor <kishor.khar...@intel.com>; Aundhe, Shirish <shirish.aun...@intel.com>; net-dev@openjdk.java.net 
Subject: RE: Patch for adding SO_REUSEPORT socket option

Hi Alan,
Here is the most recent modification of the patch. The link is available here: 
http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.11/


I know this has gone through several rounds of review already.
I do not want to stall progress, I am reasonably happy with the
changes, but I have a few comments ( that can be addressed later,
if needed ).

 - MulticastSocket.java: I would add a javadoc link to
   {@linkplain StandardSocketOptions.SO_REUSEPORT SO_REUSEPORT}

 - Maybe the socket impl set/getOption(..) methods should check
   supportedOptions().contains(name) first.  There seems to be
   many places where SO_REUSEPORT is special-cased, that could
   be replaced with a general supported options check, no?

 - How useful is it to add SO_REUSEPORT to non-listening stream
   orientated sockets ?

-Chris.




Please let us know if everything is all right.

Again, thank you very much for your help!

Thanks,
Lucy


-----Original Message-----
From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Wednesday, February 10, 2016 10: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 05/02/2016 17:27, Lu, Yingqi wrote:

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.


I've looked through the latest revision. Just a couple of small things:
In MulticastSocket then a small typo (in two places) where you have "SO_REUSPORT" instead of "SO_REUSEPORT". Also it links to setOption(int,Object) then I assume it should be setOption(SocketOption,Object).
In AbstractPlainSocketImpl and AbstractPlainDatagramSocketImpl then supportedOptions looks technically correct but there is no need to create an unmodifiableSet on each call to supportedOptions. Instead you can simply do: 

Set<SocketOption<?>> options = socketOptions; if (options == null) {
      if (isReusePortAvailable()) {
          options = new HashSet<>();
          options.addAll(super.supportedOptions());
          options.add(StandardSocketOptions.SO_REUSEPORT);
          options = Collections.unmodifiableSet(options);
      } else {
          options = super.supportedOptions();
      }
      socketOptions = options;
}
return options;
I don't see any other issues at this time and I'm happy to sponsor and help you get this in. 

-Alan.

Reply via email to