Skip to content

feat: Add option to let InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup#1271

Open
t-fitchie-meur wants to merge 7 commits into
quickfix-j:masterfrom
t-fitchie-meur:reverse-proxy-bypass
Open

feat: Add option to let InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup#1271
t-fitchie-meur wants to merge 7 commits into
quickfix-j:masterfrom
t-fitchie-meur:reverse-proxy-bypass

Conversation

@t-fitchie-meur

Copy link
Copy Markdown

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:

  • Reverse DNS lookup pre-TCP connect in IoSessionInitiator (this is fine, but may elongate session start-up time)
  • Establish TCP connection
  • Another Reverse DNS lookup in InitiatorSslFilter whilst attempting to initialize the TLS handshake
  • In the time we're blocking waiting for the Reverse DNS lookup, the acceptor times out waiting for us to initialize the TLS handshake
  • When the Reverse DNS finally times out, the acceptor has closed the TCP connection, so we get END_OF_STREAM

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

@chrjohn
chrjohn requested a review from Copilot July 1, 2026 11:00
@chrjohn

chrjohn commented Jul 1, 2026

Copy link
Copy Markdown
Member

@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 configuration.md also? I am also thinking about the naming of the setting, but that can be easily changed later on.
@the-thing any thoughts? Thanks. 😃

@chrjohn chrjohn changed the title feat: Add option to let InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup feat: Add option to let InitiatorSslFilter bypass InetSocketAddress' reverse DNS lookup Jul 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 HostResolutionStrategy with WITH_REVERSE_DNS / WITHOUT_REVERSE_DNS options.
  • Threaded the strategy through initiator connection setup and InitiatorSslFilter engine creation to choose getHostName() vs getHostString().
  • Added unit coverage for InitiatorSslFilter peer-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.

Comment thread quickfixj-core/src/main/java/quickfix/mina/initiator/IoSessionInitiator.java Outdated
@t-fitchie-meur

Copy link
Copy Markdown
Author

@chrjohn thanks for taking a look!

I've updated the configuration.md and the .html file. Let me know if there's anything else!

@chrjohn

chrjohn commented Jul 1, 2026

Copy link
Copy Markdown
Member

@t-fitchie-meur thanks. I've deleted the html doc in the meantime since it was outdated. Sorry :) I'll resolve the conflict.

@chrjohn

chrjohn commented Jul 1, 2026

Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request

@chrjohn
chrjohn requested a review from the-thing July 19, 2026 16:05
@the-thing

Copy link
Copy Markdown
Collaborator

I had a brief look and the PR looks ok itself, but...

My concern is that the scenario that is being described shouldn't normally happen unless I missed something. The reverse DNS lookup is first performed in quickfix.mina.initiator.IoSessionInitiator.ConnectTask#getSniHostName to connect to the next host in the list and later when SSL filter is installed in quickfix.mina.ssl.InitiatorSslFilter#createEngine.

java.net.InetSocketAddress#getHostName normally caches successfully resolved hostnames permanently so the resolution before connection should be instantaneous when creating SSL filter, unless DNS caching is disabled or DNS cache TTL is very small.

I'm not sure how hard is this to change, but I have a feeling that making sure that destination host is resolved only before connection is the probably the way to go.

@t-fitchie-meur

t-fitchie-meur commented Jul 20, 2026

Copy link
Copy Markdown
Author

Hey @the-thing, thanks for taking a look!

The idea here is to cover the case where the session you're trying to connect to, for whatever reason, might have reverse DNS deliberately configured not to work. When this happens, we don't have a result to cache so we just retry every time InetSocketAddress::getHostName is called.

I agree that making sure that the destination host resolution (or resolution failure) occurs pre-connection is a better solution, but this does change the behaviour of the library, and I wasn't sure how strict quickfix is with changes like this? Happy to refactor this PR in that direction if you think it's fine.

@the-thing

Copy link
Copy Markdown
Collaborator

No probs.

I agree that making sure that the destination host resolution (or resolution failure) occurs pre-connection is a better solution, but this does change the behaviour of the library, and I wasn't sure how strict quickfix is with changes like this? Happy to refactor this PR in that direction if you think it's fine.

@t-fitchie-meur

Now when I think about this. it doesn't require any change. All java.net.InetSocketAddress instances share the same cache, so successful connection to the end point will keep the successfully resolved hostname (ignoring some custom dns cache config).

The idea here is to cover the case where the session you're trying to connect to, for whatever reason, might have reverse DNS deliberately configured not to work. When this happens, we don't have a result to cache so we just retry every time InetSocketAddress::getHostName is called.

@chrjohn

The change looks good itself. This is an extra functionality, but also an additional complexity too.

@the-thing the-thing left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion would be to write an end to end test demonstrating functionality with both strategies.

Something similar to quickfix.mina.ssl.SSLCertificateTest, but it doesn't need to use SSL etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants