On 13/07/16 13:14, Svetlana Nikandrova wrote:
Chris, Daniel,
thank you for your comments.
Please see updated review:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.04/
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.04/>
Hi Svetlana,
The test looks good to me know :-)
best regards,
-- daniel
Thank you,
Svetlana
On 12.07.2016 17:25, Chris Hegarty wrote:
Svetlana,
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.03/>
The source code changes look fine. On the test...
On 11 Jul 2016, at 14:20, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
Hi Svetlana,
Have you thought of using a CountDownLatch (or some other
synchronization mechanism like e.g. Semaphore) to wait for
the server to be ready instead of:
55 int port = 0;
56 while (port == 0) {
57 Thread.sleep(500);
58 port = server.getPort();
59 }
Also serverSocket needs to be volatile.
Please remove this. It is unnecessary.
Or better yet, you could bind the server socket in
the constructor of FtpServer - which would allow you
to make serverSocket final instead of volatile,
Exactly. That is the preferred style for these kind of tests.
and then
you would no longer need to make FtpServer extend Thread.
You still need to execute the server handling code in another thread,
but FtpServer could be just a Runnable.
Additionally,
- No need to catch any Exceptions in main, just declare that main
throws Exception
- with the changes above, ‘client' may be able to move into the
try-with-resources block.
-Chris.
best regards,
-- daniel
On 07/07/16 19:06, Svetlana Nikandrova wrote:
Artem,
please see updated review:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.03/
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.03/>
Thanks,
Svetlana
On 07.07.2016 19:41, Artem Smotrakov wrote:
Hi Svetlana,
Thank you for addressing the comments below. The test looks fine to
me. Just a couple of minor comments.
1. You don't really need fields in lines 77-79.
2. try-catch block in line 86 is not really necessary as well.
It would be better to update bug subject to something more meaningful.
Artem
On 07/07/2016 08:31 AM, Svetlana Nikandrova wrote:
Artem,
thank you for suggested test improvements. Here is updated webrev:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.02/
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.02/>
Chris,
seem like you have already looked into that issue before. May be you
can find some time to review this fix?
Thank you,
Svetlana
On 06.07.2016 20:35, Artem Smotrakov wrote:
Hi Svetlana,
Thanks for addressing my comments. Could you please take a look at a
couple of comments about TestFtpClientNameListWithNull.java below?
1. lines 64-70: should "client" be closed in "finally" block? I also
think it might be better to re-throw IOExceptions instead of
ignoring them (it may hide some problems). You can just add "throws"
to main() method.
2. line 107: you close "out" in a couple of places in handelClient()
method, it might be better to add a "finally" block, and shutdown
everything there.
3. lines 176-182: you can close a client socket in handelClient().
Since it's a test for JDK 9, you can use try-with-resources like the
following:
public void handelClient(Socket cl) {
...
try (cl) {
...
}
....
(please note that it doesn't work for JDK 8)
It would reduce the code and simplify the test (you would avoid some
"try" blocks).
I am not sure that you need to close "out" if you close the socket.
4. Typo: handelClient -> handleClient
Artem
On 07/06/2016 09:11 AM, Svetlana Nikandrova wrote:
Hi Artem,
thank you for your replay. I believe I addressed all your comments.
Please see updated diff:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.01/
<http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.01/>
Thank you,
Svetlana
On 05.07.2016 20:54, Artem Smotrakov wrote:
Hi Svetlana,
I am not an official reviewer, but I have a couple comments below.
TestFtpClientNameListWithNull.java:
1. Shouldn't "commandHasArgs" be volatile? Looks like it may cause
intermittent failures. As a sanity check, you may want to run the
test in a loop to make sure it is stable.
2. You may want to make FtpServer implement AutoCloseable
(terminate() method just becomes close()). Then, you can use it in
try-with-resource block which would simplify the code (you can
avoid a couple of nested try-catch blocks).
By the way, it might be good to make sun.net.ftp.FtpClient
implement AutoCloseable as well, but probably it should be a
separate enhancement.
3. line 62-63, 66: should it be in "finally" block?
4. How many connection is FtpServer supposed to handle in this
test? If only one, it might be better to remove the "while" loop
and "done" variable to simplify the code.
5. Is it necessary to handle a client connections in a separate
thread? Avoiding too many threads would simplify the test.
6. The test ignores a couple of IOException, it might be better to
fail it they occur.
7. Why is it necessary to use daemons?
8. Please use braces for "if" statements, see Java Coding
Conventions.
FtpClient.java: please update copyright year.
Artem
On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote:
Hello,
please review this fix for bug
https://bugs.openjdk.java.net/browse/JDK-8022580
Webrev:
http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/
<http://cr.openjdk.java.net/%7Emsolovie/8022580/webrev.00/>
Description:
There is no handling for null path parameter in the method
sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that
"@param path a String containing the pathname of the directory to
list or null for the current working directory". Changeset adds
check that if param is null NLST will be sent without parameter
(no-parameter default is current directory).
JPRT tested.
Thank you,
Svetlana
