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.nikandr...@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
>>
>>
>> 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
>
