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

[Svetlana Nikandrova]

Ok, it's better to be safe than sorry. Added othervm:

http://cr.openjdk.java.net/~snikandrova/8164533/webrev.01/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.01/>

Thank you,
Svetlana

On 25.08.2016 19:02, Xuelei Fan wrote:
> 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 
> <mailto: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 
>> <https://bugs.openjdk.java.net/browse/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/82782ab2/attachment.htm>