feat: Add option to let InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup#1271
feat: Add option to let InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup#1271t-fitchie-meur wants to merge 7 commits into
InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup#1271Conversation
|
@t-fitchie-meur thanks for the PR. 👍 I will take a look. First thing that came to my mind: could you please add the config setting to |
InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup
There was a problem hiding this comment.
Pull request overview
This PR introduces a configurable way for initiator-side networking/TLS code to avoid InetSocketAddress#getHostName() reverse-DNS lookups (which can block), by routing host selection through a HostResolutionStrategy and a new session setting (ReverseDNSEnabled, defaulting to enabled for backward compatibility).
Changes:
- Added
HostResolutionStrategywithWITH_REVERSE_DNS/WITHOUT_REVERSE_DNSoptions. - Threaded the strategy through initiator connection setup and
InitiatorSslFilterengine creation to choosegetHostName()vsgetHostString(). - Added unit coverage for
InitiatorSslFilterpeer-host selection behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| quickfixj-core/src/test/java/quickfix/mina/ssl/InitiatorSslFilterTest.java | Adds tests validating peer-host selection under both host-resolution strategies. |
| quickfixj-core/src/main/java/quickfix/mina/ssl/InitiatorSslFilter.java | Uses HostResolutionStrategy to select the host passed into SSLContext#createSSLEngine. |
| quickfixj-core/src/main/java/quickfix/mina/initiator/IoSessionInitiator.java | Threads host-resolution strategy into SSL filter creation and reconnect address recreation. |
| quickfixj-core/src/main/java/quickfix/mina/initiator/AbstractSocketInitiator.java | Reads the new ReverseDNSEnabled setting and selects the appropriate strategy. |
| quickfixj-core/src/main/java/quickfix/mina/HostResolutionStrategy.java | Introduces the functional interface and default strategy implementations. |
| quickfixj-core/src/main/java/quickfix/Initiator.java | Adds the ReverseDNSEnabled settings key with Javadoc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@chrjohn thanks for taking a look! I've updated the configuration.md and the .html file. Let me know if there's anything else! |
|
@t-fitchie-meur thanks. I've deleted the html doc in the meantime since it was outdated. Sorry :) I'll resolve the conflict. |
|
@copilot resolve the merge conflicts in this pull request |
Removed comment about public API status and adjusted constructor parameters.
This is a feature to fix something that's not really a bug, but probably not an intentional feature either, and is definitely an edge case.
Summary
Currently, all of the host resolution code in quickfixj uses InetSocketAddress::getHostName, which performs a reverse DNS lookup that may block for several seconds in certain cases, such as if you are attempting to connect to an IP address that does not have a PTR DNS record. I have observed what I believe is the following race during setup of such a FIX connection:
In scenarios where we are connecting directly to an IP address, most of the time the reverse DNS lookup does not provide any functionality, so I propose that we add the option to bypass this.
Sample configuration in which the issue was discovered:
ConnectionType=initiator
SocketUseSSL=Y
EnabledProtocols=TLSv1.3
SocketConnectHost=1.2.3.4
SocketConnectPort=1234
JDK: 25
QuickFIX/J version: 3.0.1
Without the fix applied, I see the following pattern in the logs, repeating every retry interval:
MINA session created: local=/---.---.---.--/-----, remote=/---.---.---.--/-----
ERROR ... NullPointerException ... SslFilter.filterWrite ... sslHandler is null
Disconnecting: Encountered END_OF_STREAM
With the fix applied, things connect immediately, with no wait for the reverse DNS lookup.
Change safety
The reverse DNS setting defaults to enabled, so this should not introduce any regressions