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.nikandr...@oracle.com
<mailto: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
<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
