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

[Svetlana Nikandrova]

Thank you!

On 26.08.2016 16:49, Xuelei Fan wrote:
> Looks fine to me.
>
> Thanks,
> Xuelei
>
> On 8/26/2016 6:51 PM, Svetlana Nikandrova wrote:
>> 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
>>>>>>
>>>>
>>