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

[Artem Smotrakov]

Hi Xuelei,

Please see inline.

On 08/24/2016 06:10 PM, 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.
Okay, makes sense to me. I am not sure what the author meant, but I 
thought the purpose is to check if an exception is thrown when server 
accepts all connections, but then close them right away.
>
>> 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.
>>
> 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.
>
> BTW, please run the test in othervm mode.
Why do you think it's necessary here? I don't see the test modifies 
anything that may affect other tests running in the same JVM (for 
example, system properties). Am I missing something?

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