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

[Xuelei Fan]

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
>>>>>
>>>
>