RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Xuelei Fan]

Just a quick reply.

1. The close is necessary.
2. Run in othervm is safer.  Please see my comments in other replies.

If you still have questions, I will reply with more details tomorrow when I work on a PC.

Xuelei



> On Aug 25, 2016, at 11:00 PM, Svetlana Nikandrova <svetlana.nikandrova at oracle.com> wrote:
> 
> Hi Artem, Xuelei,
> 
> thank you for your comments. Please see inline.
> 
>> On 25.08.2016 4:10, Xuelei Fan wrote:
>> 
>> 
>>> On 8/25/2016 3:55 AM, Artem Smotrakov wrote: 
>>> Hi Svetlana, 
>>> 
>>> Thank you for cleaning up this test. I have a couple of comments (mostly 
>>> about the original test). 
>>> 
>>> 1. I see that the test tries to connect to a server three times, but the 
>>> server accept only first connection, and then it stops. So test cases 
>>> #2-3 fail just because the connection was refused. The original test 
>>> behaves like this. This looks like a bug to me. What do you think? 
>>> Should the server have a loop of three iterations?
>> I think it's the purpose to test behavior of the close server socket. 
> 
> Yes, the test checks that if connection was closed by server during handshake client will get exceptions from handshake, read and write. We don't need to connect 3 times to check it.
>   
>> 
>>> 2. Here is server's code: 
>>> 
>>>   95         @Override 
>>>   96         public void run() { 
>>>   97             try (Socket s = serverSocket.accept()) { 
>>>   98                 System.out.println("Server accepted connection"); 
>>>   99                 // wait a bit before closing the socket to give 
>>>  100                 // the client time to send its hello message 
>>>  101                 Thread.currentThread().sleep(100); 
>>>  102                 s.close(); 
>>>  103                 System.out.println("Server closed socket, done."); 
>>>  104             } catch (Exception e) { 
>>>  105                 throw new RuntimeException("Problem in test 
>>> execution", e); 
>>>  106             } 
>>>  107         } 
>>> 
>>> Not sure if it is a good assumption to expect that ClientHello is 
>>> received in 100 milliseconds. It might read first data, and then close 
>>> the socket. It also doesn't seem to be necessary to call close() there.
> Yes, you are right. Calling close() is unnecessary in try-with-resource block. Removed.
>> It is not expected to perform handshaking for this test.  Get connection, and immediately close the socket before handshaking, and then see what happens in client side (connect/read/write). 
>> 
>> I have the same concern that 100 MS may be an issue.  Please consider to make an improvement. 
> I also not totally happy with 100 MS assumption. As Artem suggested I've created separate issue for that improvement: JDK-8164804 
>> 
>> 
>> BTW, please run the test in othervm mode. 
> 
> I must admitted I'm confused. AFAIK othervm is forced for tests that change configuration. This test uses default one.  
> It won't kill me to add it to this test but I'd be grateful if we could clarify this for future testdev.
> 
> Thanks,
> Svetlana 
> 
>> 
>> Xuelei 
>> 
>>> Otherwise, the webrev looks good to me, but please note that I am not an 
>>> official reviewer. You may want to fix the issues above, or we can just 
>>> file a new bug. 
>>> 
>>> Artem 
>>> 
>>>> On 08/24/2016 11:21 AM, Svetlana Nikandrova wrote: 
>>>> Hello, 
>>>> 
>>>> please review this test bug fix. Test failed because of staled threads 
>>>> left after execution. 
>>>> Added try-with-resources statements to make sure test closes it's 
>>>> resources. Also as test is overall quite old-fashioned I've done some 
>>>> refactoring (hope now it looks better). 
>>>> 
>>>> JBS: 
>>>> https://bugs.openjdk.java.net/browse/JDK-8164533 
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/ 
>>>> <http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.00/> 
>>>> 
>>>> Thank you, 
>>>> Svetlana 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160826/4ceb206b/attachment.htm>