RFR: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly

[Jaikiran Pai]

On Sun, 6 Nov 2022 16:39:48 GMT, Aleksei Efimov <aefimov at openjdk.org> wrote:

> ### The Proposed Change
> 
> The proposed change updates JNDI's `DnsClient` internal implementation to use `DatagramChannel` (DC) as a replacement for `DatagramSocket` (DS).
> The main motivation behind this change is to make JNDI/DNS lookups friendly to virtual-thread environments and update its underlying implementation to use efficient `DatagramChannel` APIs.
>  The list of proposed changes:
> - Replace DS usage with DC. That includes the `DNSDatagramSocketFactory` class updates to return DC instead of DS. The factory class was renamed to `DNSDatagramChannelFactory` to reflect that.
> - Change DNS query timeouts implementation - the current version introduces a nio channels selector to implement timeouts. One selector is created for each instance of `DnsClient`.
> - Adjust query retries logic to the new implementation of timeouts.
> - Modify the `Timeout` test to create a bound UDP socket to simulate an unresponsive DNS server. Before this change, the test was using the '10.0.0.0' network address that doesn't belong to any host. The proposed change with a bound unresponsive UDP socket is better for test stability on different platforms.
> 
> 
> ### Testing
> `jdk-tier1` to `jdk-tier3 `tests are showing no failures related to the changes.
> JNDI regression and JCK tests also didn't highlight any problems with the changes. 
> 
> Also, an app performing a DNS lookup from a virtual thread context executed with the following options `--enable-preview -Djdk.tracePinnedThreads=full` showed no pinned carrier threads. Before the proposed change the following pinned stack trace was observed:
> ```    java.base/sun.nio.ch.DatagramChannelImpl.park(DatagramChannelImpl.java:486)
>     java.base/sun.nio.ch.DatagramChannelImpl.trustedBlockingReceive(DatagramChannelImpl.java:734)
>     java.base/sun.nio.ch.DatagramChannelImpl.blockingReceive(DatagramChannelImpl.java:661)
>     java.base/sun.nio.ch.DatagramSocketAdaptor.receive(DatagramSocketAdaptor.java:241) <== monitors:1
>     java.base/java.net.DatagramSocket.receive(DatagramSocket.java:714)
>     jdk.naming.dns/com.sun.jndi.dns.DnsClient.doUdpQuery(DnsClient.java:430) <== monitors:1
>     jdk.naming.dns/com.sun.jndi.dns.DnsClient.query(DnsClient.java:216)
>     jdk.naming.dns/com.sun.jndi.dns.Resolver.query(Resolver.java:81)
>     jdk.naming.dns/com.sun.jndi.dns.DnsContext.c_lookup(DnsContext.java:290)
>     java.naming/com.sun.jndi.toolkit.ctx.ComponentContext.p_lookup(ComponentContext.java:542)
>     java.naming/com.sun.jndi.toolkit.ctx.PartialCompositeContext.lookup(PartialCompositeContext.java:177)
>     java.naming/com.sun.jndi.toolkit.ctx.PartialCompositeContext.lookup(PartialCompositeContext.java:166)
>     java.naming/javax.naming.InitialContext.lookup(InitialContext.java:409)
> 
> After proposed changes - pinned threads are not detectable.

src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 470:

> 468:                 } while (timeoutLeft > MIN_TIMEOUT);
> 469:                 // no matching packet received within the timeout
> 470:                 throw new SocketTimeoutException();

It appears to me that, before the change in this PR, we used to return `null` from this method if there is a timeout. The calling code (the method `query`) would then interpret this `null` return in a couple of different ways. One of them being, skipping this server and querying any other server(s) that were known to the client instance. Now, with this change where we throw this `SocketTimeoutException`, that part of the code would behave differently from what I can see. Is this intentional to throw an exception instead of returning `null`?

-------------

PR: https://git.openjdk.org/jdk/pull/11007