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

[Svetlana Nikandrova]

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/20160825/a34e5239/attachment.htm>