On 01/12/16 14:40, Felix Yang wrote:
Hi Daniel,

   please review the new webrev:


Looks good Felix!

-- daniel

On 2016/12/1 18:58, Daniel Fuchs wrote:
Hi Felix,

Good to see one more script converted to java.

I wonder if the fact that the test use to run two separate
JVMs was significant. Hopefully it's not.
According original bug
(https://bugs.openjdk.java.net/browse/JDK-4933582), I didn't find out a
strong reason to run with two different JVMs.
But original TestHttpServe.terminate() cannot free ports on Windows,
that is why the shutdown section was adjusted. This could be a possible


The changes in TestHttpServer seem OK - this class is shared
by many other tests - so any modifications here is to be taken
with care. I trust you ran all the jdk_net tests?
Yes, tested on all platforms.


In B4933582.java:

maybe MyAuthenticator.count should be made volatile (or AtomicInteger).
I don't think it's strictly necessary as I believe our implementation
will only call it in the client thread (which is always the main thread
here) - but it may be a better idea to use AtomicInteger anyway: just
make no assumption :-)

I'd also suggest to use try-with-resource to close in
CacheImpl (int port) and CacheImpl::writeMap to make sure
the file is always properly closed.
Updated all the history codes, as you suggested
best regards,

-- daniel

On 01/12/16 09:47, Felix Yang wrote:

   please review the following patch. The code was converted from shell
script to plain Java. Since it is by-design to bind on a prior-used
port, add a few of re-tries for possible BindException.






Reply via email to