<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Ok, it's better to be safe than sorry. Added othervm:<br>
<br>
<a
href="http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.01/">http://cr.openjdk.java.net/~snikandrova/8164533/webrev.01/</a><br>
<br>
Thank you,<br>
Svetlana<br>
<br>
<div class="moz-cite-prefix">On 25.08.2016 19:02, Xuelei Fan wrote:<br>
</div>
<blockquote
cite="mid:1034AB31-F855-41CC-B354-8FCBBD69160C@Oracle.Com"
type="cite">
<meta http-equiv="Context-Type" content="text/html; charset=utf-8">
<div>Just a quick reply.</div>
<div><br>
</div>
<div>1. The close is necessary.</div>
<div>2. Run in othervm is safer. Please see my comments in other
replies.</div>
<div><br>
</div>
<div>If you still have questions, I will reply with more details
tomorrow when I work on a PC.</div>
<div><br>
</div>
<div>Xuelei</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
On Aug 25, 2016, at 11:00 PM, Svetlana Nikandrova <<a
moz-do-not-send="true"
href="mailto:svetlana.nikandrova@oracle.com">svetlana.nikandrova@oracle.com</a>>
wrote:<br>
<br>
</div>
<blockquote type="cite">
<div> 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
moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.00/">http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/</a>
<br>
<a moz-do-not-send="true" 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>
</div>
</blockquote>
</blockquote>
<br>
</body>
</html>