On Sun, 6 Nov 2022 16:39:48 GMT, Aleksei Efimov <[email protected]> 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.
-------------
PR: https://git.openjdk.org/jdk/pull/11007
