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

[Eric Wang]

Thanks David! I misunderstood your meaning of the previous mail, The 
test has been updated :)

Hi Alan,
I'm done with the test, Thank you to be my sponsor, below is the latest 
changes:
http://cr.openjdk.java.net/~ewang/8015765/webrev.03/ 
<http://cr.openjdk.java.net/%7Eewang/8015765/webrev.03/>

Thanks,
Eric
On 2013/8/15 18:07, David Holmes wrote:
> 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
>>>>
>>