Re: RFR [9] 8143554: UnsupportedOperationException is not thrown for unsupported options

2016-01-15 Thread Svetlana Nikandrova

Thank you, Alan!

If nobody has any objections I'll ask to push this fix.

Thank you,
Svetlana

On 15.01.2016 16:21, Alan Bateman wrote:



On 13/01/2016 15:48, Svetlana Nikandrova wrote:

Alan,

thank you for you replay.
Here is my updated webrev with formatting fixes.
http://cr.openjdk.java.net/~kshefov/8143554/webrev.03/ 



About getSocket() check - you are right, it is because there are 
Socket options unsupported by ServerSocket.



I looked at the latest webrev and it looks good to me.

-Alan




Re: RFR [9] 8143554: UnsupportedOperationException is not thrown for unsupported options

2016-01-15 Thread Alan Bateman



On 13/01/2016 15:48, Svetlana Nikandrova wrote:

Alan,

thank you for you replay.
Here is my updated webrev with formatting fixes.
http://cr.openjdk.java.net/~kshefov/8143554/webrev.03/ 



About getSocket() check - you are right, it is because there are 
Socket options unsupported by ServerSocket.



I looked at the latest webrev and it looks good to me.

-Alan


Re: Patch for adding SO_REUSEPORT socket option

2016-01-15 Thread Alan Bateman

On 14/01/2016 18:28, Lu, Yingqi wrote:

Hi Alan/Michael/Volker,

Here is the link to the version 7 of the patch. 
http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.07/

Please note: after the code has been uploaded, I found out I forgot to remove 
extra check of ENOPROTOOPT in the net_util_md.c. Since it took us couple days 
to upload the code each time, I think it might be a good idea to mark this as 
an open item for now. I will change it in next version for sure together with 
anything you found in the current version.


I think this looks quite good but there is still one architectural issue 
that doesn't look quite right to me.


The issue is that DatagramSocketImpl and MulticastSocket shouldn't be 
dependent on the built-in AbstractPlainSocketImpl. If you develop your 
own DatagramSocketImpl implementation then you'll quickly see the issue. 
It looks like you have it right in SocketImpl, it's just 
DatagramSocketImpl and MulticastSocket where the dependency seems to exist.


One other comment is that I don't think SocketImpl.addAdditionalOptions 
is ready to be exposed in the API (you've added it as a protected method 
as it will become part of the Java SE API). Clearly there is a need for 
impls to have a way to expand the set of socket options but it needs 
further consideration on what that API should be. The simplest is to 
just drop protected and make this a package-private method.


One other point on addAdditionalOptions is that it mutates the set of 
socket options and so is not thread safe. We'll need to come up with a 
way to do this in a safe way.


One final comment is on the Unix version of PlainSocketImpl and 
PlainDatagramSocketImpl. I'm wondering if these changes are really 
needed. This may be something Michael may have suggestions on.


-Alan