Thank you, Daniel Regards, Chris
> On 14 Sep 2018, at 6:19 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote: > > Hi Chris, > > > http://cr.openjdk.java.net/~xyin/8210695/webrev.01/ > > Looks good to me. > > > Yep, fixed as you suggested. My original though is that the stack trace > > maybe too verbose if most of runs hit NamingException, leave it to stderr > > may make the log more readable, but yes, the exception is expected and not > > considered as error as you commented. Anyway, we could adjust it later if > > necessary. > > An alternative then would be to print an additional message on > stderr - something like: > > 89 } catch (NamingException ne) { > 90 System.out.println("(" + count + "/" + REPEAT_COUNT > 91 + ") It's ok to get NamingException: " + ne); > 92 // for debug > System.err.println("Caught expected exception: " + > ne); > 93 ne.printStackTrace(); > > If you decide to go this way instead then no need for an additional > review... > > best regards, > > -- daniel > > > > On 14/09/2018 04:42, Chris Yin wrote: >> Hi, Daniel >> Thank you for the reviewing and comments, inline and update webrev as below, >> thanks >> http://cr.openjdk.java.net/~xyin/8210695/webrev.01/ >>> On 13 Sep 2018, at 5:43 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote: >>> >>> Hi Chris, >>> >>> Thanks a lot for writing this test! >>> >>> 60 serverSocket = new ServerSocket(0); >>> 69 env.put(Context.PROVIDER_URL, >>> String.format("ldap://localhost:%d/";, >>> >>> For robustness of the test I would suggest using >>> InetAddress.getLoopbackAddress() explicitly, to >>> avoid having the dummy server listen on all interfaces. >>> Listening on all interfaces has proved to be a source of >>> intermittent failures in the past, possibly due >>> to different port reuse strategies at OS level. >> Sure, fixed, thanks >>> >>> 88 System.out.println("(" + count + "/" + REPEAT_COUNT >>> 89 + ") It's ok to get NamingException: " + >>> ne); >>> 90 // for debug >>> 91 ne.printStackTrace(); >>> >>> I think it would be better in that case to print the stactrace on >>> stdout (i.e. use ne.printStackTrace(System.out)), since that >>> exception is expected and not considered as an error. >> Yep, fixed as you suggested. My original though is that the stack trace >> maybe too verbose if most of runs hit NamingException, leave it to stderr >> may make the log more readable, but yes, the exception is expected and not >> considered as error as you commented. Anyway, we could adjust it later if >> necessary. >> Regards, >> Chris >>> >>> best regards, >>> >>> -- daniel >>> >>> >>> On 13/09/2018 07:59, Chris Yin wrote: >>>> Please have a review for below new added test to cover JDK-8205330, thanks >>>> This test used dummy ldap server to simulate the scenario to send “Notice >>>> of Disconnection” after binding, it will repeat InitialDirContext() for >>>> 1000 times (normally the NPE bug should be hit less than 100 times run, >>>> but just in case), test failed with NPE if without JDK-8205330 fix change >>>> and passed after the fix. >>>> I put this test under test/jdk/com/sun/jndi/ldap/ since the bug root cause >>>> is from com.sun.jndi.ldap.LdapClient, feel free to let me know if it >>>> should be moved to other suitable folder. >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8210695 >>>> webrev: http://cr.openjdk.java.net/~xyin/8210695/webrev.00/ >>>> Regards, >>>> Chris >>> >
