Thanks for doing this Felix. I have some comments about the style and the use of AccessControlContext. Rather than list them I put an alternative version at the following location, please take a look.
http://cr.openjdk.java.net/~chegar/8156504_alt/ Note: specifically it is good practice to read off the HTTP request before closing the socket ( on the server socket ). Not doing so can lead to reset issues. -Chris. > On 2 Nov 2016, at 07:10, Felix Yang <felix.y...@oracle.com> wrote: > > Hi Amy, > > thanks for the comments. Updated webrev: > > http://cr.openjdk.java.net/~xiaofeya/8156504/webrev.01/ > > -Felix > On 2016/11/2 14:18, Amy Lu wrote: >> Good to see one more script test be changed to java, thank you Felix. >> >> I'm not official reviewer, but some minor comments. >> >> 30 * @library /lib/testlibrary >> 31 * @build jdk.testlibrary.* >> 32 * @compile LookupTest.java >> I noticed test requires testlibrary, but seems test do not actually depend >> on that lib, these lines could be removed. >> >> 77 is.close(); >> Is it better to do the close in the finally block? >> >> 128 if (serverSocket == null) { >> 129 serverSocket.close(); >> 130 } >> Typo here? >> >> Even more minor... >> - private static void addMappingToHostsFile (String host, >> - String addr, >> - String hostsFileName, >> - boolean append) >> - throws Exception { >> + private static void addMappingToHostsFile(String host, String addr, >> + String hostsFileName, boolean append) throws Exception { >> This might be reformatted automatically by IDE, but just feel previous one >> is more easy to read. >> >> Thanks, >> Amy >> >> >> On 11/2/16 10:39 AM, Felix Yang wrote: >>> Hi there, >>> >>> please review the following patch for an intermittent failing test. >>> Converted it into plain java test and avoid free port anti-pattern. >>> >>> Bug: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8156504 >>> >>> Webrev: >>> >>> http://cr.openjdk.java.net/~xiaofeya/8156504/webrev.00/ >>> >>> Thanks, >>> >>> Felix >>> >> >
