On Mon, 7 Nov 2022 07:13:04 GMT, Jaikiran Pai <j...@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
