Review request for test bug 8015765: java/nio/channels/ServerSocketChannel/AdaptServerSocket.java

[David Holmes]

On 15/08/2013 4:45 PM, Eric Wang wrote:
> Thanks David, Based on your suggestions, I updated the test again, Can
> you please help to check? Thanks!
> http://cr.openjdk.java.net/~ewang/8015765/webrev.02/
> <http://cr.openjdk.java.net/%7Eewang/8015765/webrev.02/>

You don't need two try-blocks here:

  82         try (ServerSocketChannel ssc = ServerSocketChannel.open()) {
   83             try (ServerSocket sso = ssc.socket()) {

Just

     try (ServerSocketChannel ssc = ServerSocketChannel.open());
          ServerSocket sso = ssc.socket()) {

The clientStarted change seems reasonable - but as I said I don't know 
the details of the socket code and the test.

David
-----

> Eric
> On 2013/8/15 12:29, David Holmes wrote:
>> Eric,
>>
>> Glad to hear Thread priority changes are gone :)  (Just forget Thread
>> priority even exists - it is meaningless.)
>>
>> Minor style nit:
>>
>>   82         try (ServerSocketChannel ssc =
>> ServerSocketChannel.open(); ServerSocket sso = ssc.socket();) {
>>
>> Use a line per declaration.
>>
>> I don't know what the key aspects of this test are but I suspect that
>> "clientStarted" really needs to indicate more than the fact the thread
>> has started execution of its run() method - ie it should come as close
>> as possible to the action that the main part of the test depends upon.
>>
>> David
>>
>>
>>
>> On 15/08/2013 1:22 PM, Eric Wang wrote:
>>> Hi Chris & Alan,
>>>
>>> Thanks for the review, I have updated the code based on your
>>> suggestions. Please help to review again, Thanks!
>>> http://cr.openjdk.java.net/~ewang/8015765/webrev.01/
>>> <http://cr.openjdk.java.net/%7Eewang/8015765/webrev.01/>
>>>
>>> Eric
>>> On 2013/8/13 21:48, Chris Hegarty wrote:
>>>> This test looks a little bizarre, and certainly racy. What you have
>>>> done should make the test more reliable.
>>>>
>>>> I noticed that the server socket channel is only closed if a socket is
>>>> accepted. I think it should be closed always, regardless of whether a
>>>> socket is accepted or not. Though this is not directly related to your
>>>> changes.
>>>>
>>>> -Chris
>>>>
>>>> On 08/13/2013 12:24 PM, Eric Wang wrote:
>>>>> Hi,
>>>>>
>>>>> Please help to review the fix below for the bug
>>>>> https://jbs.oracle.com/bugs/browse/JDK-8015765.
>>>>> http://cr.openjdk.java.net/~ewang/8015765/webrev.00/
>>>>>
>>>>> As the test is depended on thread schedule, it may fail
>>>>> intermittently.The fix is to adjust thread priority, and increase
>>>>> thread
>>>>> sleep duration, to make it more stable.
>>>>>
>>>>> Thanks,
>>>>> Eric
>>>
>