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

[Aleksei Efimov]

On Mon, 7 Nov 2022 07:13:04 GMT, Jaikiran Pai <jpai 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 185:
> 
>> 183:     public void close() {
>> 184:         try {
>> 185:             udpChannelSelector.close();
> 
> Do you think we should now maintain a `closed` boolean flag in this class to keep track of whether this underlying selector has been closed? 
> 
> The javadoc of `Selector.close()` states that any subsequent use of the selector will throw a `ClosedSelectorException`. A `ClosedSelectorException` is a `RuntimeException`, so if a closed `DnsClient` gets used (for example a `query()` gets triggered) then from what I can see it will end up propagating this `ClosedSelectorException` out of these class methods instead of the declared compile time exceptions.
> 
> Maintaining a `closed` flag could allow us to use that flag to check in these methods (where we use the selector) and throw a more appropriate exception.

Good point - planning to address the closed selector in the `blockingReceive` method by calling `udpChannelSelector.isOpen()`.

> 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`?

Thanks for spotting that, Jai.
You're correct that in its current version the fix changed an old logic of `doUdpQuery`/`query` methods:
Before this change the method was returning `null` not when a receive is timed out but when an unmatched packet is received. Socket timeout exceptions thrown by `DatagramSocket.receive` were caught in `query` method.
After the proposed change the `doUdpQuery` method is throwing `SocketTimeoutException` for both cases (timeout and unmatched packets) - that needs to be changed to comply with old logic. Will address it in an upcoming changeset.

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

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