GeezerVison

I haven't forgotten about this, but as mentioned, it'll take some more time until I can review it properly.
Thanks for your work so far!

No worries, take your time. Thanks for the update.

I'll review in more detail later, but as a first change, please undo the new *sendAndReceive methods in the interfaces and their corresponding usage in SimpleResolver (as well as the new proxy constructor there) and DefaultIoClient. Instead, please create a dedicated IoClientFactory, e.g. Socks5ProxyIoClientFactory and Socks5ProxyIoClient, this can then internally handle the proxy.

I undid the changes in SimpleResolver, DefaultIoClient and *sendAndReceive.

I also added a Socks5ProxyIoClientFactory. Now, you can set it up like following:

InetSocketAddress proxyAddress = new InetSocketAddress(InetAddress.getByName("<socks5-host>"), 1080);
String socks5User = "<user>";
String socks5Password = "<password>";
Socks5ProxyConfig config = new Socks5ProxyConfig(proxyAddress, socks5User, socks5Password);
res.setIoClientFactory(new Socks5ProxyIoClientFactory(config));

I couldn't figure out how to add the SOCKS5 transactions with the NioClient. I implemented it like an event loop in the Socks5ProxyIoClientFactory and added the read and write operations in the attachment of the channel keys. I tried to implement the timeout handling with a ScheduledExecutorService, but I think it is not ready, yet.

Now it's fully async. IPv6 worked with 2001:4860:4860::8888 for me. It also reuses the connection without repeating the SOCKS5 handshake again.

Hi @ibauersachs. I suggested a huge amount of code additions and I also have changed a lot since the requested changes. I think, I should give some more descriptions.

Initially, I did not understand the NioClient back then. So, I attempted to implement it in a way that suited the handling of the SOCKS5 handshake. My concept was to start with an event loop that uses the Selector as thread safe queue and store the read/write logic as Runnables in the attachment of the SelectionKey. A new Runnable is set in the attachment at the end of the previous one to chain the Runnables in the right order. Or the CompletableFuture is completed. It should also work together with a worker pool to execute the Runnables in a multithreaded way.

Then later I realized I could not sync up the close operations for the implementation of timeouts into the event loop through the Selector. I didn't want to use selector.select(timeout) and opted for a solution with the java.util.concurrent.ScheduledExecutorService. I think, there is still a problem with synchronizing the close operation with the connect, read and write operations in a thread safe way. Now I think, I should write a method that handles connect, read and write and wrap it around a synchronized(tcpSelectionKey) block in Socks5ProxyTcpIoClient. Similar for Socks5ProxyUdpIoClient.

To reuse the connections and avoiding the repetition of the SOCKS handshake, I defined private static final Map<String, Map<String, Socks5Proxy>> socks5ConnectionPool = new ConcurrentHashMap<>(); Initially, I used a simple Map<String, Socks5Proxy>, but I realized that I needed to track the state of the TCP connection to prevent the SOCKS handshake and read/write operations from being disrupted by concurrent usage. Therefore, I decided to use a nested Map structure to track the connections per remote address in the outer Map and manage multiple connections for the same remote host in a the inner Map. This structure might require a cleanup strategy to close connections that aren't used for a longer time. I used the SelectionKey registration and deregistration from the Selector to track whether a connection is usable or if a new connection to the host needs to be established. This way I prevent that concurrent use messes up the UDP/TCP transaction of a connection.

What do you think?

Hi, sorry for not getting back to you yet. I haven't had time to thoroughly review this PR again, and as you probably figured out, the NIO stuff is nasty.

At first glance, there's a lot of duplication between the existing IO client classes and what you wrote now. I wonder if there couldn't be more shared code, especially the selector thread and timeout handling. The IO clients are currently package-private, so there can be changes to the API if needed.
The general idea with the map of connections etc. is probably correct.

It would also be good if there were some unit/integration tests. You could use Testcontainers to run a real proxy to test against.

Hi. Thanks for your quick reply.

Yes. I now believe that the logic for the SOCKS proxy should be built on top of the NioClients. I can't use the current NioTcpClient because of this part ChannelState channel = channelMap.computeIfAbsent(new ChannelKey(local, remote), ...). For SOCKS, the remote would always be the proxy.
It would suggest to have the selector thread, timeout handling and connection pooling in the IoClientFactory and passing a connection handler to the IoClients. I will try something like this.

The current TcpIoClient implementation currently makes a hard assumption that it's talking directly with DNS servers, also with the prefix-length expected in each reply. You can also try introducing an intermediate class if it helps:

         NioClient 
             |
     AbstractTcpClient
     |               |
DnsTcpClient    SocksClient

Going back to the IoClientFactory will provide the challenge of keeping the public interface stable.

That might help. I try it out.

Hallo @ibauersachs. Thanks for your input. I appreciate it.

I integrated the transactions with the IoClients. Selector and timeout handling is now shared. Also I added 3 basic tests with Testcontainers (ComposeContainer).

The builds for this pull request are currently failing. Probably, Docker is not set up in the build containers.

Error:  Errors: 
Error:  org.xbill.DNS.io.SimpleSocksTest.null
Error:    Run 1: SimpleSocksTest.setUp:27 � IllegalState Could not find a valid Docker environment. Please see logs and check configuration
Error:    Run 2: SimpleSocksTest.setUp:27 � ContainerFetch Can't get Docker image: RemoteDockerImage(imageName=docker:24.0.2, imagePullPolicy=DefaultPullPolicy(), 

It works fine on my server.

[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 38.84 s -- in org.xbill.DNS.io.SimpleSocksTest

It seems that on Mac, there is an issue with routing UDP traffic into the Docker container. I do not believe this is related to the proper functioning of the SOCKS interactions.

[ERROR] Errors: 
[ERROR] org.xbill.DNS.io.SimpleSocksTest.testAuth
[ERROR]   Run 1: SimpleSocksTest.testAuth » PortUnreachable

It works fine when I try it from a test container in the same Docker network.

The introduction of intermediate classes worked well.

                                NioClient 
                            /              \                                
                NioTcpHandler               NioUdpHandler
           /        |          \             /       \          
NioTcpClient NioSocksTcpClient  NioSocksUdpClient    NioUdpClient
                          \        /        
                        NioSocksHandler

For the NioSocksHandler, I now handle I/O using futures and the provided Transaction classes. I use a future for the entire SOCKS handshake and one for each transaction.

For the NioSocksTcpClient, I moved the logic from NioTcpClient to the NioTcpHandler. I introduced boolean attributes to perform operations for the SOCKS interactions when necessary. I added a future as an attribute for the SOCKS handshake on the ChannelState to manage the reuse of the TCP channels without redoing the SOCKS handshake.

For the NioSocksUdpClient, I encountered an issue where the SOCKS proxy only accepts UDP interactions from the same port after the first interaction. To address this, I wrote NioSocksUdpAssociateChannelPool, that keeps track of the established SOCKS UDP associations and handles multiple SOCKS connections per local+remote address.

I plan to add more tests and further improve and simplify the NioSocksUdpAssociateChannelPool to avoid needing a new SOCKS connection for each remote address. And I'm looking for a way to reduce the log output of Testcontainers.

It would be nice to get a short review and your opinion on the pull request.

Thanks! I'll need a while to dig through all of this.

As for the integration tests, you could add a tag to only run them on the stage that also runs Sonar (i.e. Ubuntu and currently Java 21). Docker might need to be installed beforehand, you can do that as an actions step with apt-get install.

Also, can you please rebase the branch on top of master?

I rebased the branch on top of master.