On Mon, 7 Nov 2022 19:28:56 GMT, Aleksei Efimov <aefi...@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.
>
> Aleksei Efimov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
Changes requested by dfuchs (Reviewer).
src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DNSDatagramChannelFactory.java
line 279:
> 277: } catch (IOException x) {
> 278: // try again until maxtries == 0;
> 279: }
That can be simplified now:
DatagramChannel dc = (family != null)
? DatagramChannel.open(family)
: DatagramChannel.open();
try {
dc.bind(new InetSocketAddress(port));
lastport = getLocalPort(dc);
if (!recycled) history.add(port);
return dc;
} catch (IOException x) {
dc.close();
// try again until maxtries == 0;
}
There's probably no need to retry if the open call fails, as the next call to
`open` is likely to fail too. So if `open` throws we should probably let it
propagate upwards - (and declare openRandom() to throw IOException).
-------------
PR: https://git.openjdk.org/jdk/pull/11007
