On 02/11/16 11:18, Chris Hegarty wrote:
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.
+ 1 for getting rid of the AccessControlContext.
I was not very comfortable with that either.
cheers,
-- daniel
-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
