[11u] RFR (S): 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Severin Gehwolf]

Hi Christoph,

On Mon, 2020-08-31 at 15:14 +0000, Langer, Christoph wrote:
> Hi Goetz,
> 
> good catch. Seems like I overlooked that one.
> 
> Here's an updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8151678.11u.1/

I had a look at this for 11u approval and this jumped out at me:

src/jdk.naming.ldap/share/classes/com/sun/jndi/ldap/dns/LdapDnsProviderService.java

 107         synchronized (LOCK) {
 108             Iterator<LdapDnsProvider> iterator = providers.iterator();
 109             while (result == null && iterator.hasNext()) {
 110                 result = iterator.next().lookupEndpoints(url, envCopy)
 111                         .filter(r -> r.getEndpoints().isEmpty())
 112                         .orElse(null);
 113             }

Line 111 inverses the filter from the original change[1]. This looks
wrong.

Thanks,
Severin

[1] https://hg.openjdk.java.net/jdk/jdk/rev/1def54255e93#l2.59

> Best regards
> Christoph
> 
> 
> > -----Original Message-----
> > From: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> > Sent: Montag, 31. August 2020 16:07
> > To: Langer, Christoph <christoph.langer at sap.com>; jdk-updates-
> > dev at openjdk.java.net
> > Subject: RE: [11u] RFR (S): 8151678:
> > com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on
> > DeadServerNoTimeoutTest is incorrect
> > 
> > Hi Christoph,
> > 
> > I had a look at your change.
> > One question:
> > 
> > Why do you use size() and not emtpy() here?
> > 
> > LdapDnsProviderService.java:
> > @@ -99,14 +101,16 @@
> >      LdapDnsProviderResult lookupEndpoints(String url, Hashtable<?,?> env)
> >          throws NamingException
> >      {
> > -        Iterator<LdapDnsProvider> iterator = providers.iterator();
> > -        Hashtable<?, ?> envCopy = new Hashtable<>(env);
> >          LdapDnsProviderResult result = null;
> > +        Hashtable<?, ?> envCopy = new Hashtable<>(env);
> > 
> > -        while (result == null && iterator.hasNext()) {
> > -            result = iterator.next().lookupEndpoints(url, envCopy)
> > -                    .filter(r -> r.getEndpoints().size() > 0)
> > -                    .orElse(null);
> > +        synchronized (LOCK) {
> > +            Iterator<LdapDnsProvider> iterator = providers.iterator();
> > +            while (result == null && iterator.hasNext()) {
> > +                result = iterator.next().lookupEndpoints(url, envCopy)
> > +                        .filter(r -> r.getEndpoints().size() > 0)
> > <=======
> > +                        .orElse(null);
> > +            }
> >          }
> > 
> >          return result;
> > 
> > Besides that it looks good.
> > 
> > Best regards,
> >   Goetz.
> > 
> > 
> > 
> > 
> > -----Original Message-----
> > From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> On
> > Behalf Of Langer, Christoph
> > Sent: Montag, 31. August 2020 11:31
> > To: jdk-updates-dev at openjdk.java.net
> > Subject: [CAUTION] [11u] RFR (S): 8151678:
> > com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on
> > DeadServerNoTimeoutTest is incorrect
> > 
> > Hi,
> > 
> > may I please have a review for the backport of test fix JDK-8151678
> > "com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on
> > DeadServerNoTimeoutTest is incorrect" as follow-up for JDK-8160768 (Add
> > capability to custom resolve host/domain names within the default JNDI
> > LDAP provider).
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8151678
> > Original change: https://hg.openjdk.java.net/jdk/jdk/rev/1def54255e93
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8151678.11u.0/
> > 
> > I had the following Rejects:
> > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.j
> > ava:
> >     needed manual resolve, also copyright year was different
> > LdapDnsProviderService.java:
> >     File is in a different location in 11u, so I needed to manually resolve it:
> > 
> > java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java
> > ->
> > jdk.naming.ldap/share/classes/com/sun/jndi/ldap/dns/LdapDnsProviderSer
> > vice.java
> > ProblemList.txt:
> >     Needed a manual resolve
> > 
> > Thanks
> > Christoph