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

[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: RFR [9] 8143554: UnsupportedOperationException is not thrown for unsupported options

[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

[Svetlana Nikandrova]

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.


Thank you,
Svetlana

On 30.12.2015 11:15, Alan Bateman wrote:


The update to PlainSocketImpl and PlainDatagramSocketImpl look okay, 
just a minor nit that "if(" should be "if (" to keep the code consistent.


For SocketImpl then I assume the getSocket() check is because these 
are Socket rather than ServerSocket options.


There are few formatting issues in the test that would be good to fix 
up before this is pushed, otherwise the test looks okay to me.


-Alan

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

[Alan Bateman]

On 23/12/2015 11:22, Svetlana Nikandrova wrote:

Little remainder.

On 19.12.2015 1:32, Svetlana Nikandrova wrote:

Alan, Michael,

thank you for your feedback.
As you suggested I moved checking code down to the implementation. 
Please see updated webrev:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.02/ 



Thank you,
Svetlana

The update to PlainSocketImpl and PlainDatagramSocketImpl look okay, 
just a minor nit that "if(" should be "if (" to keep the code consistent.


For SocketImpl then I assume the getSocket() check is because these are 
Socket rather than ServerSocket options.


There are few formatting issues in the test that would be good to fix up 
before this is pushed, otherwise the test looks okay to me.


-Alan

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

[Svetlana Nikandrova]

Little remainder.

On 19.12.2015 1:32, Svetlana Nikandrova wrote:

Alan, Michael,

thank you for your feedback.
As you suggested I moved checking code down to the implementation. 
Please see updated webrev:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.02/ 



Thank you,
Svetlana

On 08.12.2015 16:51, Alan Bateman wrote:



On 08/12/2015 13:44, Svetlana Nikandrova wrote:

Hi Alan,

thank you for your replay. Please let me explain a little.
For example ServerSocket and Socket have different supported options 
set, but the same SocketImpl under the hood.
Yes, SocketImpl's setOptions() and getOptions() can be modified to 
add additional check for the actual socket type that encloses that 
socket implementation, but I believe this will intricate method's 
logic and tangle the dependencies. I like it how it was done in jdk 
8 - clean and simple - so I think it's a good idea to maintain that 
approach in jdk 9 as well.
I think this needs a closer look and I hope Michael will have cycles. 
As things currently stands then the SocketImpl methods are specified 
to throw UOE if called with unsupported operations. Yes, this might 
mean the implementation needs to know whether it's for a connecting 
or listening socket.


-Alan

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

[Svetlana Nikandrova]

Alan, Michael,

thank you for your feedback.
As you suggested I moved checking code down to the implementation. 
Please see updated webrev:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.02/ 



Thank you,
Svetlana

On 08.12.2015 16:51, Alan Bateman wrote:



On 08/12/2015 13:44, Svetlana Nikandrova wrote:

Hi Alan,

thank you for your replay. Please let me explain a little.
For example ServerSocket and Socket have different supported options 
set, but the same SocketImpl under the hood.
Yes, SocketImpl's setOptions() and getOptions() can be modified to 
add additional check for the actual socket type that encloses that 
socket implementation, but I believe this will intricate method's 
logic and tangle the dependencies. I like it how it was done in jdk 8 
- clean and simple - so I think it's a good idea to maintain that 
approach in jdk 9 as well.
I think this needs a closer look and I hope Michael will have cycles. 
As things currently stands then the SocketImpl methods are specified 
to throw UOE if called with unsupported operations. Yes, this might 
mean the implementation needs to know whether it's for a connecting or 
listening socket.


-Alan

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

[Alan Bateman]

I'm sure Michael will look at this but I have a question - shouldn't 
SocketImpl throw UOE for this case? I'm just wondering if checking the 
supported options in setOption/getOption is just covering up an issue 
with the SocketImpl methods.


-Alan


On 08/12/2015 12:36, Svetlana Nikandrova wrote:

Little reminder.

On 03.12.2015 16:06, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and 
setOption sockets' methods similar way as it was done prior jdk 9 in 
Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. I 
added test to the ProblemList.


Thank you,
Svetlana

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

[Svetlana Nikandrova]

Hi Alan,

thank you for your replay. Please let me explain a little.
For example ServerSocket and Socket have different supported options 
set, but the same SocketImpl under the hood.
Yes, SocketImpl's setOptions() and getOptions() can be modified to add 
additional check for the actual socket type that encloses that socket 
implementation, but I believe this will intricate method's logic and 
tangle the dependencies. I like it how it was done in jdk 8 - clean and 
simple - so I think it's a good idea to maintain that approach in jdk 9 
as well.


Thank you,
Svetlana

On 08.12.2015 15:56, Alan Bateman wrote:


I'm sure Michael will look at this but I have a question - shouldn't 
SocketImpl throw UOE for this case? I'm just wondering if checking the 
supported options in setOption/getOption is just covering up an issue 
with the SocketImpl methods.


-Alan


On 08/12/2015 12:36, Svetlana Nikandrova wrote:

Little reminder.

On 03.12.2015 16:06, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and 
setOption sockets' methods similar way as it was done prior jdk 9 in 
Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. 
I added test to the ProblemList.


Thank you,
Svetlana

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

[Svetlana Nikandrova]

Hi Artem,

thank you for your comments. You are right in both cases. I changed to 
"else if" and removed IAE exception from main().


Please see updated review:

http://cr.openjdk.java.net/~kshefov/8143554/webrev.01/ 



Thank you,
Svetlana

On 03.12.2015 17:02, Artem Smotrakov wrote:

Hi Svetlana,

I'll leave the mail review to official reviewers, a couple of minor 
comments about your test.


It seems to work fine, but you may want to use "else if" in 
UnsupportedOptionsTest.getOption() method because "Unsupported socket 
type" error can occure in case of supported socket type.


Not sure that IllegalAccessException can be thrown in main() method.

Artem

On 12/03/2015 04:06 PM, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and 
setOption sockets' methods similar way as it was done prior jdk 9 in 
Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. I 
added test to the ProblemList.


Thank you,
Svetlana

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

[Artem Smotrakov]

Hi Svetlana,

I'll leave the mail review to official reviewers, a couple of minor 
comments about your test.


It seems to work fine, but you may want to use "else if" in 
UnsupportedOptionsTest.getOption() method because "Unsupported socket 
type" error can occure in case of supported socket type.


Not sure that IllegalAccessException can be thrown in main() method.

Artem

On 12/03/2015 04:06 PM, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and setOption 
sockets' methods similar way as it was done prior jdk 9 in Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. I 
added test to the ProblemList.


Thank you,
Svetlana

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

[Svetlana Nikandrova]

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and setOption 
sockets' methods similar way as it was done prior jdk 9 in Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. I 
added test to the ProblemList.


Thank you,
Svetlana