8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly #11007
Conversation
|
👋 Welcome back aefimov! A progress list of the required criteria for merging this PR into |
|
@AlekseiEfimov The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| String allQuietUrl = "dns://" + HOST + ":" + PORT; | ||
| // Create a DatagramSocket and bind it to the loopback address to simulate | ||
| // UDP DNS server that doesn't respond | ||
| DatagramSocket ds = new DatagramSocket( |
There was a problem hiding this comment.
Hello Aleksei, perhaps close this socket in a finally block in this test?
| @@ -159,6 +181,10 @@ protected void finalize() { | |||
| private Object queuesLock = new Object(); | |||
|
|
|||
| public void close() { | |||
| try { | |||
| udpChannelSelector.close(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point - planning to address the closed selector in the blockingReceive method by calling udpChannelSelector.isOpen().
There was a problem hiding this comment.
Addressed it by catching ClosedSelectorException and wrapping it into JNDI's CommunicationException in 55dd0a4
| return null; // no matching packet received within the timeout | ||
| } while (timeoutLeft > MIN_TIMEOUT); | ||
| // no matching packet received within the timeout | ||
| throw new SocketTimeoutException(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, already addressed in 55dd0a4
| int pktTimeout = (timeout * (1 << retry)); | ||
| udpSocket.send(opkt); | ||
| udpChannel.send(opkt, target); |
There was a problem hiding this comment.
FYI: When a DatagramChannel is connected then you can use write to send the datagram, you don't need to specify the target address again when sending.
| if (family != null) { | ||
| DatagramChannel c = DatagramChannel.open(family); | ||
| try { | ||
| DatagramSocket s = c.socket(); | ||
| s.bind(new InetSocketAddress(port)); | ||
| lastport = s.getLocalPort(); | ||
| c.bind(new InetSocketAddress(port)); | ||
| lastport = getLocalPort(c); | ||
| if (!recycled) history.add(port); | ||
| return s; | ||
| return c; | ||
| } catch (Throwable x) { | ||
| c.close(); | ||
| throw x; | ||
| } | ||
| } | ||
| DatagramSocket s = new DatagramSocket(port); | ||
| lastport = s.getLocalPort(); | ||
| var dc = DatagramChannel.open(); | ||
| dc.bind(new InetSocketAddress(port)); | ||
| lastport = getLocalPort(dc); | ||
| if (!recycled) history.add(port); | ||
| return s; | ||
| return dc; | ||
| } catch (IOException x) { | ||
| // try again until maxtries == 0; | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Thanks for suggestion - openRandom looks much simplier, no duplicated code and in-sync with openDefault. I agree that it is reasonable not to retry open call in failure case, and propagate IOException. Changed in fa7ef18
| return null; // no matching packet received within the timeout | ||
| } while (timeoutLeft > MIN_TIMEOUT); | ||
| // no matching packet received within the timeout | ||
| throw new SocketTimeoutException(); |
|
@AlekseiEfimov This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 21 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thank you Aleksei for the changes. The latest revision looks good to me. |
|
/integrate |
|
Going to push as commit 9ef7852.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@AlekseiEfimov Pushed as commit 9ef7852. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The Proposed Change
The proposed change updates JNDI's
DnsClientinternal implementation to useDatagramChannel(DC) as a replacement forDatagramSocket(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
DatagramChannelAPIs.The list of proposed changes:
DNSDatagramSocketFactoryclass updates to return DC instead of DS. The factory class was renamed toDNSDatagramChannelFactoryto reflect that.DnsClient.Timeouttest 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-tier1tojdk-tier3tests 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=fullshowed no pinned carrier threads. Before the proposed change the following pinned stack trace was observed:After proposed changes - pinned threads are not detectable.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11007/head:pull/11007$ git checkout pull/11007Update a local copy of the PR:
$ git checkout pull/11007$ git pull https://git.openjdk.org/jdk pull/11007/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11007View PR using the GUI difftool:
$ git pr show -t 11007Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11007.diff