<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Artem, Xuelei,<br>
<br>
thank you for your comments. Please see inline.<br>
<br>
<div class="moz-cite-prefix">On 25.08.2016 4:10, Xuelei Fan wrote:<br>
</div>
<blockquote
cite="mid:101564da-f528-72f2-c0dc-eaf6dcbf5dbc@oracle.com"
type="cite">
<br>
<br>
On 8/25/2016 3:55 AM, Artem Smotrakov wrote:
<br>
<blockquote type="cite">Hi Svetlana,
<br>
<br>
Thank you for cleaning up this test. I have a couple of comments
(mostly
<br>
about the original test).
<br>
<br>
1. I see that the test tries to connect to a server three times,
but the
<br>
server accept only first connection, and then it stops. So test
cases
<br>
#2-3 fail just because the connection was refused. The original
test
<br>
behaves like this. This looks like a bug to me. What do you
think?
<br>
Should the server have a loop of three iterations?
<br>
<br>
</blockquote>
I think it's the purpose to test behavior of the close server
socket.
<br>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:101564da-f528-72f2-c0dc-eaf6dcbf5dbc@oracle.com"
type="cite">
<br>
<blockquote type="cite">2. Here is server's code:
<br>
<br>
95 @Override
<br>
96 public void run() {
<br>
97 try (Socket s = serverSocket.accept()) {
<br>
98 System.out.println("Server accepted
connection");
<br>
99 // wait a bit before closing the socket to
give
<br>
100 // the client time to send its hello
message
<br>
101 Thread.currentThread().sleep(100);
<br>
102 s.close();
<br>
103 System.out.println("Server closed socket,
done.");
<br>
104 } catch (Exception e) {
<br>
105 throw new RuntimeException("Problem in test
<br>
execution", e);
<br>
106 }
<br>
107 }
<br>
<br>
Not sure if it is a good assumption to expect that ClientHello
is
<br>
received in 100 milliseconds. It might read first data, and then
close
<br>
the socket. It also doesn't seem to be necessary to call close()
there.
<br>
<br>
</blockquote>
</blockquote>
Yes, you are right. Calling close() is unnecessary in
try-with-resource block. Removed.<br>
<blockquote
cite="mid:101564da-f528-72f2-c0dc-eaf6dcbf5dbc@oracle.com"
type="cite">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).
<br>
<br>
I have the same concern that 100 MS may be an issue. Please
consider to make an improvement.
<br>
</blockquote>
I also not totally happy with 100 MS assumption. As Artem suggested
I've created separate issue for that improvement: <a
class="issue-link" data-issue-key="JDK-8164804"
href="https://bugs.openjdk.java.net/browse/JDK-8164804"
id="key-val" rel="4898594">JDK-8164804</a>
<blockquote
cite="mid:101564da-f528-72f2-c0dc-eaf6dcbf5dbc@oracle.com"
type="cite">
<br>
BTW, please run the test in othervm mode.
<br>
</blockquote>
<br>
I must admitted I'm confused. AFAIK othervm is forced for tests that
change configuration. This test uses default one. <br>
It won't kill me to add it to this test but I'd be grateful if we
could clarify this for future testdev.<br>
<br>
Thanks,<br>
Svetlana <br>
<br>
<blockquote
cite="mid:101564da-f528-72f2-c0dc-eaf6dcbf5dbc@oracle.com"
type="cite">
<br>
Xuelei
<br>
<br>
<blockquote type="cite">Otherwise, the webrev looks good to me,
but please note that I am not an
<br>
official reviewer. You may want to fix the issues above, or we
can just
<br>
file a new bug.
<br>
<br>
Artem
<br>
<br>
On 08/24/2016 11:21 AM, Svetlana Nikandrova wrote:
<br>
<blockquote type="cite">Hello,
<br>
<br>
please review this test bug fix. Test failed because of staled
threads
<br>
left after execution.
<br>
Added try-with-resources statements to make sure test closes
it's
<br>
resources. Also as test is overall quite old-fashioned I've
done some
<br>
refactoring (hope now it looks better).
<br>
<br>
JBS:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8164533">https://bugs.openjdk.java.net/browse/JDK-8164533</a>
<br>
Webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/">http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.00/"><http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.00/></a>
<br>
<br>
Thank you,
<br>
Svetlana
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>