From 2f953e44572662103df81872c7fc27570dc6ca29 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 26 Mar 2020 16:25:13 -0700 Subject: [PATCH] grpc-js: Don't wait for TXT record to return DNS lookup result --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/resolver-dns.ts | 204 +++++++++++++------------ packages/grpc-js/src/server.ts | 2 + packages/grpc-js/test/test-resolver.ts | 43 ++++-- 4 files changed, 137 insertions(+), 114 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index aa2d6bfd..011fe79b 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "0.7.3", + "version": "0.7.4", "description": "gRPC Library for Node - pure JS implementation", "homepage": "https://grpc.io/", "repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js", diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index ebfd16dc..4da295e1 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -69,34 +69,7 @@ const DNS_REGEX = /^(?:dns:)?(?:\/\/(?:[a-zA-Z0-9-]+\.?)+\/)?((?:[a-zA-Z0-9-]+\. */ const DEFAULT_PORT = '443'; -/** - * Get a promise that always resolves with either the result of the function - * or the error if it failed. - * @param fn - */ -function resolvePromisify( - fn: ( - arg: TArg, - callback: (error: TError | null, result: TResult) => void - ) => void -): (arg: TArg) => Promise { - return arg => - new Promise((resolve, reject) => { - fn(arg, (error, result) => { - if (error) { - resolve(error); - } else { - resolve(result); - } - }); - }); -} - -const resolveTxtPromise = resolvePromisify< - string, - string[][], - NodeJS.ErrnoException ->(dns.resolveTxt); +const resolveTxtPromise = util.promisify(dns.resolveTxt); const dnsLookupPromise = util.promisify(dns.lookup); /** @@ -156,11 +129,11 @@ class DnsResolver implements Resolver { private readonly ipResult: SubchannelAddress[] | null; private readonly dnsHostname: string | null; private readonly port: string | null; - /* The promise results here contain, in order, the A record, the AAAA record, - * and either the TXT record or an error if TXT resolution failed */ - private pendingResultPromise: Promise< - [dns.LookupAddress[], string[][] | NodeJS.ErrnoException] - > | null = null; + private pendingLookupPromise: Promise | null = null; + private pendingTxtPromise: Promise | null = null; + private latestLookupResult: TcpSubchannelAddress[] | null = null; + private latestServiceConfig: ServiceConfig | null = null; + private latestServiceConfigError: StatusObject | null = null; private percentage: number; private defaultResolutionError: StatusObject; constructor(private target: string, private listener: ResolverListener) { @@ -189,7 +162,7 @@ class DnsResolver implements Resolver { /** * If the target is an IP address, just provide that address as a result. - * Otherwise, initiate A, AAAA, and TXT + * Otherwise, initiate A, AAAA, and TXT lookups */ private startResolution() { if (this.ipResult !== null) { @@ -200,87 +173,116 @@ class DnsResolver implements Resolver { return; } if (this.dnsHostname !== null) { + /* We clear out latestLookupResult here to ensure that it contains the + * latest result since the last time we started resolving. That way, the + * TXT resolution handler can use it, but only if it finishes second. We + * don't clear out any previous service config results because it's + * better to use a service config that's slightly out of date than to + * revert to an effectively blank one. */ + this.latestLookupResult = null; const hostname: string = this.dnsHostname; /* We lookup both address families here and then split them up later * because when looking up a single family, dns.lookup outputs an error * if the name exists but there are no records for that family, and that * error is indistinguishable from other kinds of errors */ - const addressResult = dnsLookupPromise(hostname, { all: true }); - /* We handle the TXT query promise differently than the others because - * the name resolution attempt as a whole is a success even if the TXT - * lookup fails */ - const txtResult = resolveTxtPromise(hostname); - this.pendingResultPromise = Promise.all([addressResult, txtResult]); - this.pendingResultPromise.then( - ([addressList, txtRecord]) => { - this.pendingResultPromise = null; - const ip4Addresses: dns.LookupAddress[] = addressList.filter( - addr => addr.family === 4 - ); - const ip6Addresses: dns.LookupAddress[] = addressList.filter(addr => addr.family === 6); - const allAddresses: TcpSubchannelAddress[] = mergeArrays( - ip4Addresses, - ip6Addresses - ).map(addr => ({ host: addr.address, port: +this.port! })); - const allAddressesString: string = - '[' + - allAddresses.map(addr => addr.host + ':' + addr.port).join(',') + - ']'; - trace( - 'Resolved addresses for target ' + - this.target + - ': ' + - allAddressesString - ); - if (allAddresses.length === 0) { - this.listener.onError(this.defaultResolutionError); - return; - } - let serviceConfig: ServiceConfig | null = null; - let serviceConfigError: StatusObject | null = null; - if (txtRecord instanceof Error) { - serviceConfigError = { + this.pendingLookupPromise = dnsLookupPromise(hostname, { all: true }); + this.pendingLookupPromise.then(addressList => { + this.pendingLookupPromise = null; + const ip4Addresses: dns.LookupAddress[] = addressList.filter( + addr => addr.family === 4 + ); + const ip6Addresses: dns.LookupAddress[] = addressList.filter(addr => addr.family === 6); + this.latestLookupResult = mergeArrays( + ip6Addresses, + ip4Addresses + ).map(addr => ({ host: addr.address, port: +this.port! })); + const allAddressesString: string = + '[' + + this.latestLookupResult.map(addr => addr.host + ':' + addr.port).join(',') + + ']'; + trace( + 'Resolved addresses for target ' + + this.target + + ': ' + + allAddressesString + ); + if (this.latestLookupResult.length === 0) { + this.listener.onError(this.defaultResolutionError); + return; + } + /* If the TXT lookup has not yet finished, both of the last two + * arguments will be null, which is the equivalent of getting an + * empty TXT response. When the TXT lookup does finish, its handler + * can update the service config by using the same address list */ + this.listener.onSuccessfulResolution( + this.latestLookupResult, + this.latestServiceConfig, + this.latestServiceConfigError + ); + }, + err => { + trace( + 'Resolution error for target ' + + this.target + + ': ' + + (err as Error).message + ); + this.pendingLookupPromise = null; + this.listener.onError(this.defaultResolutionError); + }); + /* If there already is a still-pending TXT resolution, we can just use + * that result when it comes in */ + if (this.pendingTxtPromise === null) { + /* We handle the TXT query promise differently than the others because + * the name resolution attempt as a whole is a success even if the TXT + * lookup fails */ + this.pendingTxtPromise = resolveTxtPromise(hostname); + this.pendingTxtPromise.then(txtRecord => { + this.pendingTxtPromise = null; + try { + this.latestServiceConfig = extractAndSelectServiceConfig( + txtRecord, + this.percentage + ); + } catch (err) { + this.latestServiceConfigError = { code: Status.UNAVAILABLE, - details: 'TXT query failed', + details: 'Parsing service config failed', metadata: new Metadata(), }; - } else { - try { - serviceConfig = extractAndSelectServiceConfig( - txtRecord, - this.percentage - ); - } catch (err) { - serviceConfigError = { - code: Status.UNAVAILABLE, - details: 'Parsing service config failed', - metadata: new Metadata(), - }; - } } - this.listener.onSuccessfulResolution( - allAddresses, - serviceConfig, - serviceConfigError - ); - }, - err => { - trace( - 'Resolution error for target ' + - this.target + - ': ' + - (err as Error).message - ); - this.pendingResultPromise = null; - this.listener.onError(this.defaultResolutionError); - } - ); + if (this.latestLookupResult !== null) { + /* We rely here on the assumption that calling this function with + * identical parameters will be essentialy idempotent, and calling + * it with the same address list and a different service config + * should result in a fast and seamless switchover. */ + this.listener.onSuccessfulResolution( + this.latestLookupResult, + this.latestServiceConfig, + this.latestServiceConfigError + ) + } + }, err => { + this.latestServiceConfigError = { + code: Status.UNAVAILABLE, + details: 'TXT query failed', + metadata: new Metadata(), + }; + if (this.latestLookupResult !== null) { + this.listener.onSuccessfulResolution( + this.latestLookupResult, + this.latestServiceConfig, + this.latestServiceConfigError + ) + } + }); + } } } updateResolution() { trace('Resolution update requested for target ' + this.target); - if (this.pendingResultPromise === null) { + if (this.pendingLookupPromise === null) { this.startResolution(); } } diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 62ed68ab..fc48f3cf 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -321,6 +321,8 @@ export class Server { const resolverListener: ResolverListener = { onSuccessfulResolution: (addressList, serviceConfig, serviceConfigError) => { + // We only want one resolution result. Discard all future results + resolverListener.onSuccessfulResolution = () => {} if (addressList.length === 0) { callback(new Error(`No addresses resolved for port ${port}`), 0); return; diff --git a/packages/grpc-js/test/test-resolver.ts b/packages/grpc-js/test/test-resolver.ts index 7d7343c0..7f4900aa 100644 --- a/packages/grpc-js/test/test-resolver.ts +++ b/packages/grpc-js/test/test-resolver.ts @@ -38,6 +38,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -71,6 +73,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -104,6 +108,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -129,6 +135,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -154,6 +162,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -179,6 +189,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert(addressList.length > 0); done(); }, @@ -197,6 +209,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -224,6 +238,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -249,6 +265,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => @@ -278,6 +296,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert(addressList.length > 0); done(); }, @@ -290,16 +310,6 @@ describe('Name Resolver', () => { }); it('Should resolve gRPC interop servers', done => { let completeCount = 0; - function done2(error?: Error) { - if (error) { - done(error); - } else { - completeCount += 1; - if (completeCount === 2) { - done(); - } - } - } const target1 = 'grpc-test.sandbox.googleapis.com'; const target2 = 'grpc-test4.sandbox.googleapis.com'; const listener: resolverManager.ResolverListener = { @@ -309,10 +319,15 @@ describe('Name Resolver', () => { serviceConfigError: StatusObject | null ) => { assert(addressList.length > 0); - done2(); + completeCount += 1; + if (completeCount === 2) { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; + done(); + } }, onError: (error: StatusObject) => { - done2(new Error(`Failed with status ${error.details}`)); + done(new Error(`Failed with status ${error.details}`)); }, }; const resolver1 = resolverManager.createResolver(target1, listener); @@ -330,6 +345,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr => !isTcpSubchannelAddress(addr) && addr.path === 'socket' @@ -352,6 +369,8 @@ describe('Name Resolver', () => { serviceConfig: ServiceConfig | null, serviceConfigError: StatusObject | null ) => { + // Only handle the first resolution result + listener.onSuccessfulResolution = () => {}; assert( addressList.some( addr =>