From 35f723bacfd38ffccb84cc0ed86d0b847eba47c3 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 21 Jan 2025 14:32:01 -0500 Subject: [PATCH 1/3] feat(NODE-6451): retry DNS timeout on SRV and TXT lookup --- src/connection_string.ts | 28 ++- src/mongo_client.ts | 8 + .../dns_seedlist.test.ts | 162 ++++++++++++++++++ 3 files changed, 194 insertions(+), 4 deletions(-) create mode 100644 test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts diff --git a/src/connection_string.ts b/src/connection_string.ts index d50a1fcfa94..322d5ca51d2 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -52,6 +52,27 @@ const LB_REPLICA_SET_ERROR = 'loadBalanced option not supported with a replicaSe const LB_DIRECT_CONNECTION_ERROR = 'loadBalanced option not supported when directConnection is provided'; +function retryDNSTimeoutFor(api: 'resolveSrv'): (a: string) => Promise; +function retryDNSTimeoutFor(api: 'resolveTxt'): (a: string) => Promise; +function retryDNSTimeoutFor( + api: 'resolveSrv' | 'resolveTxt' +): (a: string) => Promise { + return async function dnsReqRetryTimeout(lookupAddress: string) { + try { + return await dns.promises[api](lookupAddress); + } catch (firstDNSError) { + if (firstDNSError.code === dns.TIMEOUT) { + return await dns.promises[api](lookupAddress); + } else { + throw firstDNSError; + } + } + }; +} + +const resolveSrv = retryDNSTimeoutFor('resolveSrv'); +const resolveTxt = retryDNSTimeoutFor('resolveTxt'); + /** * Lookup a `mongodb+srv` connection string, combine the parts and reparse it as a normal * connection string. @@ -67,14 +88,13 @@ export async function resolveSRVRecord(options: MongoOptions): Promise implements * This means the time to setup the `MongoClient` does not count against `timeoutMS`. * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * + * @remarks + * The driver will look up corresponding SRV and TXT records if the connection string starts with `mongodb+srv://`. + * If those look ups throw a DNS Timeout error, the driver will retry the look up once. + * * @see docs.mongodb.org/manual/reference/connection-string/ */ async connect(): Promise { @@ -727,6 +731,10 @@ export class MongoClient extends TypedEventEmitter implements * @remarks * The programmatically provided options take precedence over the URI options. * + * @remarks + * The driver will look up corresponding SRV and TXT records if the connection string starts with `mongodb+srv://`. + * If those look ups throw a DNS Timeout error, the driver will retry the look up once. + * * @see https://www.mongodb.com/docs/manual/reference/connection-string/ */ static async connect(url: string, options?: MongoClientOptions): Promise { diff --git a/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts b/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts new file mode 100644 index 00000000000..636a2895e28 --- /dev/null +++ b/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts @@ -0,0 +1,162 @@ +import { expect } from 'chai'; +import * as dns from 'dns'; +import * as sinon from 'sinon'; + +import { MongoClient } from '../../mongodb'; + +const metadata: MongoDBMetadataUI = { requires: { topology: '!single' } }; + +// This serves as a placeholder for _whatever_ node.js may throw. We only rely upon `.code` +class DNSTimeoutError extends Error { + code = 'ETIMEOUT'; +} +// This serves as a placeholder for _whatever_ node.js may throw. We only rely upon `.code` +class DNSSomethingError extends Error { + code = undefined; +} + +const CONNECTION_STRING = `mongodb+srv://test1.test.build.10gen.cc`; +// 27018 localhost.test.build.10gen.cc. +// 27017 localhost.test.build.10gen.cc. + +describe('DNS timeout errors', () => { + let client: MongoClient; + + beforeEach(async function () { + client = new MongoClient(CONNECTION_STRING, { serverSelectionTimeoutMS: 2000, tls: false }); + }); + + afterEach(async function () { + sinon.restore(); + await client.close(); + }); + + const restoreDNS = + api => + async (...args) => { + sinon.restore(); + return await dns.promises[api](...args); + }; + + describe('when SRV record look up times out', () => { + beforeEach(() => { + sinon + .stub(dns.promises, 'resolveSrv') + .onFirstCall() + .rejects(new DNSTimeoutError()) + .onSecondCall() + .callsFake(restoreDNS('resolveSrv')); + }); + + afterEach(async function () { + sinon.restore(); + }); + + it('retries timeout error', metadata, async () => { + await client.connect(); + }); + }); + + describe('when TXT record look up times out', () => { + beforeEach(() => { + sinon + .stub(dns.promises, 'resolveTxt') + .onFirstCall() + .rejects(new DNSTimeoutError()) + .onSecondCall() + .callsFake(restoreDNS('resolveTxt')); + }); + + afterEach(async function () { + sinon.restore(); + }); + + it('retries timeout error', metadata, async () => { + await client.connect(); + }); + }); + + describe('when SRV record look up times out twice', () => { + beforeEach(() => { + sinon + .stub(dns.promises, 'resolveSrv') + .onFirstCall() + .rejects(new DNSTimeoutError()) + .onSecondCall() + .rejects(new DNSTimeoutError()) + .onThirdCall() + .callsFake(restoreDNS('resolveSrv')); + }); + + afterEach(async function () { + sinon.restore(); + }); + + it('throws timeout error', metadata, async () => { + const error = await client.connect().catch(error => error); + expect(error).to.be.instanceOf(DNSTimeoutError); + }); + }); + + describe('when TXT record look up times out twice', () => { + beforeEach(() => { + sinon + .stub(dns.promises, 'resolveTxt') + .onFirstCall() + .rejects(new DNSTimeoutError()) + .onSecondCall() + .rejects(new DNSTimeoutError()) + .onThirdCall() + .callsFake(restoreDNS('resolveTxt')); + }); + + afterEach(async function () { + sinon.restore(); + }); + + it('throws timeout error', metadata, async () => { + const error = await client.connect().catch(error => error); + expect(error).to.be.instanceOf(DNSTimeoutError); + }); + }); + + describe('when SRV record look up throws a non-timeout error', () => { + beforeEach(() => { + sinon + .stub(dns.promises, 'resolveSrv') + .onFirstCall() + .rejects(new DNSSomethingError()) + .onSecondCall() + .callsFake(restoreDNS('resolveSrv')); + }); + + afterEach(async function () { + sinon.restore(); + }); + + it('throws that error', metadata, async () => { + const error = await client.connect().catch(error => error); + expect(error).to.be.instanceOf(DNSSomethingError); + }); + }); + + describe('when TXT record look up throws a non-timeout error', () => { + beforeEach(() => { + sinon + .stub(dns.promises, 'resolveTxt') + .onFirstCall() + .rejects(new DNSSomethingError()) + .onSecondCall() + .callsFake(restoreDNS('resolveTxt')); + }); + + afterEach(async function () { + sinon.restore(); + }); + + it('throws that error', metadata, async () => { + const error = await client.connect().catch(error => error); + expect(error).to.be.instanceOf(DNSSomethingError); + }); + }); +}); From 4bfeb664d23b9b2ae48c6265843a086499a439cf Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 23 Jan 2025 11:01:11 -0500 Subject: [PATCH 2/3] check call count --- .../dns_seedlist.test.ts | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts b/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts index 636a2895e28..51daf681578 100644 --- a/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts +++ b/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts @@ -16,17 +16,17 @@ class DNSSomethingError extends Error { } const CONNECTION_STRING = `mongodb+srv://test1.test.build.10gen.cc`; -// 27018 localhost.test.build.10gen.cc. -// 27017 localhost.test.build.10gen.cc. describe('DNS timeout errors', () => { let client: MongoClient; + let stub; beforeEach(async function () { client = new MongoClient(CONNECTION_STRING, { serverSelectionTimeoutMS: 2000, tls: false }); }); afterEach(async function () { + stub = undefined; sinon.restore(); await client.close(); }); @@ -40,7 +40,7 @@ describe('DNS timeout errors', () => { describe('when SRV record look up times out', () => { beforeEach(() => { - sinon + stub = sinon .stub(dns.promises, 'resolveSrv') .onFirstCall() .rejects(new DNSTimeoutError()) @@ -54,12 +54,13 @@ describe('DNS timeout errors', () => { it('retries timeout error', metadata, async () => { await client.connect(); + expect(stub).to.have.been.calledTwice; }); }); describe('when TXT record look up times out', () => { beforeEach(() => { - sinon + stub = sinon .stub(dns.promises, 'resolveTxt') .onFirstCall() .rejects(new DNSTimeoutError()) @@ -73,12 +74,13 @@ describe('DNS timeout errors', () => { it('retries timeout error', metadata, async () => { await client.connect(); + expect(stub).to.have.been.calledTwice; }); }); describe('when SRV record look up times out twice', () => { beforeEach(() => { - sinon + stub = sinon .stub(dns.promises, 'resolveSrv') .onFirstCall() .rejects(new DNSTimeoutError()) @@ -100,7 +102,7 @@ describe('DNS timeout errors', () => { describe('when TXT record look up times out twice', () => { beforeEach(() => { - sinon + stub = sinon .stub(dns.promises, 'resolveTxt') .onFirstCall() .rejects(new DNSTimeoutError()) @@ -117,12 +119,13 @@ describe('DNS timeout errors', () => { it('throws timeout error', metadata, async () => { const error = await client.connect().catch(error => error); expect(error).to.be.instanceOf(DNSTimeoutError); + expect(stub).to.have.been.calledTwice; }); }); describe('when SRV record look up throws a non-timeout error', () => { beforeEach(() => { - sinon + stub = sinon .stub(dns.promises, 'resolveSrv') .onFirstCall() .rejects(new DNSSomethingError()) @@ -137,12 +140,13 @@ describe('DNS timeout errors', () => { it('throws that error', metadata, async () => { const error = await client.connect().catch(error => error); expect(error).to.be.instanceOf(DNSSomethingError); + expect(stub).to.have.been.calledOnce; }); }); describe('when TXT record look up throws a non-timeout error', () => { beforeEach(() => { - sinon + stub = sinon .stub(dns.promises, 'resolveTxt') .onFirstCall() .rejects(new DNSSomethingError()) @@ -157,6 +161,7 @@ describe('DNS timeout errors', () => { it('throws that error', metadata, async () => { const error = await client.connect().catch(error => error); expect(error).to.be.instanceOf(DNSSomethingError); + expect(stub).to.have.been.calledOnce; }); }); }); From 182b4f70eaea95a66c6a3a90dba8c116ce7c73f1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 23 Jan 2025 13:20:57 -0500 Subject: [PATCH 3/3] remove third call condition --- .../initial-dns-seedlist-discovery/dns_seedlist.test.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts b/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts index 51daf681578..9ccc87fab30 100644 --- a/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts +++ b/test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts @@ -85,9 +85,7 @@ describe('DNS timeout errors', () => { .onFirstCall() .rejects(new DNSTimeoutError()) .onSecondCall() - .rejects(new DNSTimeoutError()) - .onThirdCall() - .callsFake(restoreDNS('resolveSrv')); + .rejects(new DNSTimeoutError()); }); afterEach(async function () { @@ -97,6 +95,7 @@ describe('DNS timeout errors', () => { it('throws timeout error', metadata, async () => { const error = await client.connect().catch(error => error); expect(error).to.be.instanceOf(DNSTimeoutError); + expect(stub).to.have.been.calledTwice; }); }); @@ -107,9 +106,7 @@ describe('DNS timeout errors', () => { .onFirstCall() .rejects(new DNSTimeoutError()) .onSecondCall() - .rejects(new DNSTimeoutError()) - .onThirdCall() - .callsFake(restoreDNS('resolveTxt')); + .rejects(new DNSTimeoutError()); }); afterEach(async function () {