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