Hey @chrislukkk,

first of all, thank you for the work on this. Multi Subnet failover is a great feature to have as part of tedious. 👍

There is a few things that need to be fixed up before this will be ready to be merged:

• From what I understand, the default behavior for a TDS connection when connecting to a hostname is that the hostname is resolved via DNS and for each returned record, a connection attempt is performed. These attempts are performed serially.
• If multiSubnetFailover is true, these connections should happen in parallel, the way you implemented here.
• I think we should ignore the multiSubnetFailover option in case that the host that is being connected to is an ip address, otherwise the code will try to lookup an IP address in DNS, which won't work.

Do you feel up to the task of extending this PR to implement these missing functionalities?

Thanks @chrislukkk for the fix-ups.

Hi @arthurschreiber,

Thanks a lot for the thorough code review!

Regarding to the things you mentioned:

a. You are correct that TDS client by default connects sequentially if there are multiple ip addresses bind to an host, and multisubnetfailover make the workflow in parallel.

To support sequential connection, however, will change the default connect behavior of tedious, because tedious by default use socket.connect(in net.js), which internally calls dns.lookup to find the first found IP record and connect to it(not finding all IP record and connect sequentially). To correct that we will use need to use same logic of multiconnect for connection regardless whether multisubnetfailover is set. We were worried that will be too much for this change.

b. I will add the logic to check if the host is an IP address. There is an npm package called ip which does the ip format check already. Is it ok to use other npm package or should I just copy the regex over? There is an net.isIP function we can use to detect whether host is an ip address

Hey @chrislukkk

Thanks for all the fixups. As you already found out, there is an isIP method in the core net module. This is looking pretty good, I'll probably merge this as-is ASAP and then fix up some smaller issues in some later PRs. Thank you so much for this! ❤️

Hi @arthurschreiber, is there any update one the process of this PR? Thank you!

Hey @chrislukkk

I'm really sorry this has been dragged on so long. I'll see if I can carve out a bit of free time to review this thoroughly and get this change integrated. 👍

Hey @chrislukkk,

I'm closing this in favor of #362. Please take a look and let me know what you think!