From 141dfeb790968359a060b4e812f35ae078c32b96 Mon Sep 17 00:00:00 2001 From: Simon Woolf Date: Fri, 21 Aug 2020 18:10:58 +0100 Subject: [PATCH 01/15] Channel#watchConnectivityState: handle infinite deadlines correctly Per https://grpc.github.io/grpc/node/grpc.html#~Deadline: "If it is a finite number, it is treated as a number of milliseconds since the Unix Epoch. If it is Infinity, the deadline will never be reached. If it is -Infinity, the deadline has already passed." --- packages/grpc-js/src/channel.ts | 40 +++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/packages/grpc-js/src/channel.ts b/packages/grpc-js/src/channel.ts index 8c369192..3195ebc2 100644 --- a/packages/grpc-js/src/channel.ts +++ b/packages/grpc-js/src/channel.ts @@ -120,7 +120,7 @@ export interface Channel { interface ConnectivityStateWatcher { currentState: ConnectivityState; - timer: NodeJS.Timeout; + timer: NodeJS.Timeout | null; callback: (error?: Error) => void; } @@ -417,7 +417,9 @@ export class ChannelImplementation implements Channel { const watchersCopy = this.connectivityStateWatchers.slice(); for (const watcherObject of watchersCopy) { if (newState !== watcherObject.currentState) { - clearTimeout(watcherObject.timer); + if(watcherObject.timer) { + clearTimeout(watcherObject.timer); + } this.removeConnectivityStateWatcher(watcherObject); watcherObject.callback(); } @@ -452,25 +454,29 @@ export class ChannelImplementation implements Channel { deadline: Date | number, callback: (error?: Error) => void ): void { - const deadlineDate: Date = - deadline instanceof Date ? deadline : new Date(deadline); - const now = new Date(); - if (deadlineDate <= now) { - process.nextTick( - callback, - new Error('Deadline passed without connectivity state change') - ); - return; - } - const watcherObject = { - currentState, - callback, - timer: setTimeout(() => { + let timer = null; + if(deadline !== Infinity) { + const deadlineDate: Date = + deadline instanceof Date ? deadline : new Date(deadline); + const now = new Date(); + if (deadline === -Infinity || deadlineDate <= now) { + process.nextTick( + callback, + new Error('Deadline passed without connectivity state change') + ); + return; + } + timer = setTimeout(() => { this.removeConnectivityStateWatcher(watcherObject); callback( new Error('Deadline passed without connectivity state change') ); - }, deadlineDate.getTime() - now.getTime()), + }, deadlineDate.getTime() - now.getTime()) + } + const watcherObject = { + currentState, + callback, + timer }; this.connectivityStateWatchers.push(watcherObject); } From c536178c675aa3527aa8cd366a274009ed7a050c Mon Sep 17 00:00:00 2001 From: WK Date: Sat, 29 Aug 2020 19:05:42 +0800 Subject: [PATCH 02/15] Fixed connectivity to Google PubSub over proxy Latest version has caused @google-cloud/pubsub fail to connect over a proxy connection. Snapshot of error: 2020-08-29T10:52:45.340Z | proxy | Successfully connected to pubsub.googleapis.c om:443 through proxy 172.16.52.252:443 2020-08-29T10:52:45.370Z | subchannel | 172.16.52.252:443 CONNECTING -> TRANSIEN T_FAILURE 2020-08-29T10:52:45.372Z | pick_first | CONNECTING -> TRANSIENT_FAILURE 2020-08-29T10:52:45.373Z | resolving_load_balancer | dns:172.16.52.252:443 CONNE CTING -> TRANSIENT_FAILURE 2020-08-29T10:52:45.375Z | channel | Pick result: TRANSIENT_FAILURE subchannel: undefined status: 14 No connection established 2020-08-29T10:52:45.377Z | call_stream | [11] cancelWithStatus code: 14 details: "No connection established" 2020-08-29T10:52:45.379Z | call_stream | [11] ended with status: code=14 details ="No connection established" 2020-08-29T10:52:45.381Z | connectivity_state | dns:172.16.52.252:443 CONNECTING -> TRANSIENT_FAILURE Before proposed fix: static getDefaultAuthority(target) { return target.path; // this returns "pubsub.googleapis.com:443" } After proposed fix: static getDefaultAuthority(target) { const hostPort = uri_parser_1.splitHostPort(target.path); // target.path is "pubsub.googleapis.com:443" if (hostPort !== null) { return hostPort.host; // this returns "pubsub.googleapis.com" } else { throw new Error(`Failed to parse target ${uri_parser_1.uriToString(target)}`); } } --- packages/grpc-js/src/resolver-dns.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 2db8a5e4..3f28254b 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -268,7 +268,13 @@ class DnsResolver implements Resolver { * @param target */ static getDefaultAuthority(target: GrpcUri): string { - return target.path; + const hostPort = uri_parser_1.splitHostPort(target.path); + if (hostPort !== null) { + return hostPort.host; + } + else { + throw new Error(`Failed to parse target ${uri_parser_1.uriToString(target)}`); + } } } From 6f3db6f4d89c8fcd308e700374c69aadf864b19f Mon Sep 17 00:00:00 2001 From: WK Date: Sun, 30 Aug 2020 14:49:54 +0800 Subject: [PATCH 03/15] Update http_proxy.ts --- packages/grpc-js/src/http_proxy.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/src/http_proxy.ts b/packages/grpc-js/src/http_proxy.ts index 8411e117..e3fdee05 100644 --- a/packages/grpc-js/src/http_proxy.ts +++ b/packages/grpc-js/src/http_proxy.ts @@ -215,8 +215,10 @@ export function getProxiedConnection( * connection to a TLS connection. * This is a workaround for https://github.com/nodejs/node/issues/32922 * See https://github.com/grpc/grpc-node/pull/1369 for more info. */ - const remoteHost = getDefaultAuthority(parsedTarget); - + const targetPath = getDefaultAuthority(parsedTarget); + const hostPort = splitHostPort(targetPath); + const remoteHost = (hostPort !== null) ? hostPort.host : targetPath; + const cts = tls.connect( { host: remoteHost, From 08350ec0efe84eabbb3e950fcdce6d470fe6c555 Mon Sep 17 00:00:00 2001 From: WK Date: Sun, 30 Aug 2020 14:52:20 +0800 Subject: [PATCH 04/15] Update subchannel.ts --- packages/grpc-js/src/subchannel.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index 736562a6..87805246 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -467,11 +467,13 @@ export class Subchannel { * if a connection is successfully established through the proxy. * If the proxy is not used, these connectionOptions are discarded * anyway */ - connectionOptions.servername = getDefaultAuthority( + const targetPath = getDefaultAuthority( parseUri(this.options['grpc.http_connect_target'] as string) ?? { path: 'localhost', } ); + const hostPort = splitHostPort(targetPath); + connectionOptions.servername = (hostPort !== null) ? hostPort.host : targetPath; } } } From 6a99983ed110433cedd432c8f020b123aaaf4fd7 Mon Sep 17 00:00:00 2001 From: WK Date: Sun, 30 Aug 2020 14:56:23 +0800 Subject: [PATCH 05/15] Undo changes. --- packages/grpc-js/src/resolver-dns.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 3f28254b..2db8a5e4 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -268,13 +268,7 @@ class DnsResolver implements Resolver { * @param target */ static getDefaultAuthority(target: GrpcUri): string { - const hostPort = uri_parser_1.splitHostPort(target.path); - if (hostPort !== null) { - return hostPort.host; - } - else { - throw new Error(`Failed to parse target ${uri_parser_1.uriToString(target)}`); - } + return target.path; } } From 148b273f196ee5bdfc4a6459d07a5d6440edc0c9 Mon Sep 17 00:00:00 2001 From: WK Date: Tue, 1 Sep 2020 01:28:53 +0800 Subject: [PATCH 06/15] Update http_proxy.ts --- packages/grpc-js/src/http_proxy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/http_proxy.ts b/packages/grpc-js/src/http_proxy.ts index e3fdee05..84e29a50 100644 --- a/packages/grpc-js/src/http_proxy.ts +++ b/packages/grpc-js/src/http_proxy.ts @@ -217,7 +217,7 @@ export function getProxiedConnection( * See https://github.com/grpc/grpc-node/pull/1369 for more info. */ const targetPath = getDefaultAuthority(parsedTarget); const hostPort = splitHostPort(targetPath); - const remoteHost = (hostPort !== null) ? hostPort.host : targetPath; + const remoteHost = hostPort?.host ?? targetPath; const cts = tls.connect( { From 7fc0035f7f84646269fd391f9cb80ab404f5319a Mon Sep 17 00:00:00 2001 From: WK Date: Tue, 1 Sep 2020 01:30:35 +0800 Subject: [PATCH 07/15] Update subchannel.ts --- packages/grpc-js/src/subchannel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index 87805246..a8c81df8 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -473,7 +473,7 @@ export class Subchannel { } ); const hostPort = splitHostPort(targetPath); - connectionOptions.servername = (hostPort !== null) ? hostPort.host : targetPath; + connectionOptions.servername = hostPort?.host ?? : targetPath; } } } From 158d0dd99f9997e2ad34cec2fc756ae21bddc5f9 Mon Sep 17 00:00:00 2001 From: WK Date: Tue, 1 Sep 2020 01:40:14 +0800 Subject: [PATCH 08/15] Update subchannel.ts --- packages/grpc-js/src/subchannel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index a8c81df8..3be6dd0a 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -473,7 +473,7 @@ export class Subchannel { } ); const hostPort = splitHostPort(targetPath); - connectionOptions.servername = hostPort?.host ?? : targetPath; + connectionOptions.servername = hostPort?.host ?? targetPath; } } } From 38e988ea03bd76ab4002e69903d43d3fe1b31fd9 Mon Sep 17 00:00:00 2001 From: Slavo Vojacek Date: Mon, 31 Aug 2020 20:34:14 +0100 Subject: [PATCH 09/15] fix(grpc-js): Add support for impl type to server.addService --- packages/grpc-js/src/server.ts | 11 ++++++++--- packages/grpc-js/test/test-server.ts | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 8d8952e8..673e913d 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -144,9 +144,14 @@ export class Server { throw new Error('Not implemented. Use addService() instead'); } - addService( - service: ServiceDefinition, - implementation: UntypedServiceImplementation + addService< + ImplementationType extends Record< + string, + UntypedHandleCall + > = UntypedServiceImplementation + >( + service: ServiceDefinition, + implementation: ImplementationType ): void { if (this.started === true) { throw new Error("Can't add a service to a started server."); diff --git a/packages/grpc-js/test/test-server.ts b/packages/grpc-js/test/test-server.ts index 434efbbc..f8b13b30 100644 --- a/packages/grpc-js/test/test-server.ts +++ b/packages/grpc-js/test/test-server.ts @@ -181,7 +181,7 @@ describe('Server', () => { const server = new Server(); assert.throws(() => { - server.addService({}, dummyImpls); + server.addService({} as any, dummyImpls); }, /Cannot add an empty service to a server/); }); From 5e42be1b3485949ccecd6b9068d603b298177e17 Mon Sep 17 00:00:00 2001 From: Algin Maduro Date: Tue, 1 Sep 2020 12:59:07 +0200 Subject: [PATCH 10/15] fix: preserve original error code if present --- packages/grpc-js/src/server-call.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/src/server-call.ts b/packages/grpc-js/src/server-call.ts index 3ff4c55e..7c740013 100644 --- a/packages/grpc-js/src/server-call.ts +++ b/packages/grpc-js/src/server-call.ts @@ -743,7 +743,10 @@ export class Http2ServerCallStream< // Ignore any remaining messages when errors occur. this.bufferedMessages.length = 0; - err.code = Status.INTERNAL; + if(!err.code){ + err.code = Status.INTERNAL; + } + readable.emit('error', err); } From aaee068a6972d9ff18db8fc19c460b02b1a0b5ff Mon Sep 17 00:00:00 2001 From: Algin Maduro Date: Tue, 1 Sep 2020 13:28:23 +0200 Subject: [PATCH 11/15] fix: add addition check if the provided code is valid gRPC code --- packages/grpc-js/src/server-call.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js/src/server-call.ts b/packages/grpc-js/src/server-call.ts index 7c740013..fad0eddb 100644 --- a/packages/grpc-js/src/server-call.ts +++ b/packages/grpc-js/src/server-call.ts @@ -739,15 +739,24 @@ export class Http2ServerCallStream< } else { this.messagesToPush.push(deserialized); } - } catch (err) { + } catch (error) { // Ignore any remaining messages when errors occur. this.bufferedMessages.length = 0; - if(!err.code){ - err.code = Status.INTERNAL; + if ( + !( + 'code' in error && + typeof error.code === 'number' && + Number.isInteger(error.code) && + error.code >= Status.OK && + error.code <= Status.UNAUTHENTICATED + ) + ) { + // The error code is not a valid gRPC code so its being overwritten. + error.code = Status.INTERNAL; } - - readable.emit('error', err); + + readable.emit('error', error); } this.isPushPending = false; From b2d89820a3ba920fc5a4ea35dad63a857b546346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20=C5=A0mol=C3=ADk?= Date: Tue, 1 Sep 2020 14:24:25 +0200 Subject: [PATCH 12/15] Fix ClientOptions types Remove index signature from ChannelOptions to fix intersection error described in #1558 which causes issues on using ClientOptions direct fields with TypeScript. Removing of index signature required few minor changes: - adding few constant types that were used throughout the app - using `as const` assertion in xds-client - using not-so-great type cast in channelOptionsEqual Alternative solution would be removing the index signature from ChannelOptions explicitly in ClientOptions definition, which is not trivial and probably calls for a generic type helper. See: https://github.com/grpc/grpc-node/issues/1558 Fixes: #1558 --- packages/grpc-js/src/channel-options.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/src/channel-options.ts b/packages/grpc-js/src/channel-options.ts index 8c4756ec..f316a58a 100644 --- a/packages/grpc-js/src/channel-options.ts +++ b/packages/grpc-js/src/channel-options.ts @@ -33,7 +33,9 @@ export interface ChannelOptions { 'grpc.max_send_message_length'?: number; 'grpc.max_receive_message_length'?: number; 'grpc.enable_http_proxy'?: number; - [key: string]: string | number | undefined; + 'grpc.http_connect_target'?: string; + 'grpc.http_connect_creds'?: string; + [key: string]: any; } /** From 0d1d5a12fa1e3ee444db6929e9f7707b6bde5bc1 Mon Sep 17 00:00:00 2001 From: Slavo Vojacek Date: Wed, 2 Sep 2020 16:05:10 +0100 Subject: [PATCH 13/15] chore(Typings): Update types --- packages/grpc-js/src/server.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 673e913d..9b5f3af8 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -145,10 +145,9 @@ export class Server { } addService< - ImplementationType extends Record< - string, - UntypedHandleCall - > = UntypedServiceImplementation + ImplementationType extends { + readonly [index in keyof ImplementationType]: UntypedHandleCall; + } >( service: ServiceDefinition, implementation: ImplementationType @@ -166,7 +165,7 @@ export class Server { throw new Error('addService() requires two objects as arguments'); } - const serviceKeys = Object.keys(service); + const serviceKeys = Object.keys(service) as Array; if (serviceKeys.length === 0) { throw new Error('Cannot add an empty service to a server'); @@ -194,13 +193,13 @@ export class Server { let impl; if (implFn === undefined && typeof attrs.originalName === 'string') { - implFn = implementation[attrs.originalName]; + implFn = implementation[attrs.originalName as keyof ImplementationType]; } if (implFn !== undefined) { - impl = implFn.bind(implementation); + impl = implFn.bind(implementation as any); } else { - impl = getDefaultHandler(methodType, name); + impl = getDefaultHandler(methodType, name as string); } const success = this.register( From fba1ee0cc398c63d0a3e960e8418eb8dd3dbe30c Mon Sep 17 00:00:00 2001 From: Slavo Vojacek Date: Wed, 2 Sep 2020 20:32:31 +0100 Subject: [PATCH 14/15] fix(grpc): Fix typings --- packages/grpc-js/src/server.ts | 10 +++++----- packages/grpc-js/test/test-server.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 9b5f3af8..64c17a02 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -146,7 +146,7 @@ export class Server { addService< ImplementationType extends { - readonly [index in keyof ImplementationType]: UntypedHandleCall; + [key: string]: any } >( service: ServiceDefinition, @@ -165,7 +165,7 @@ export class Server { throw new Error('addService() requires two objects as arguments'); } - const serviceKeys = Object.keys(service) as Array; + const serviceKeys = Object.keys(service); if (serviceKeys.length === 0) { throw new Error('Cannot add an empty service to a server'); @@ -193,13 +193,13 @@ export class Server { let impl; if (implFn === undefined && typeof attrs.originalName === 'string') { - implFn = implementation[attrs.originalName as keyof ImplementationType]; + implFn = implementation[attrs.originalName]; } if (implFn !== undefined) { - impl = implFn.bind(implementation as any); + impl = implFn.bind(implementation); } else { - impl = getDefaultHandler(methodType, name as string); + impl = getDefaultHandler(methodType, name); } const success = this.register( diff --git a/packages/grpc-js/test/test-server.ts b/packages/grpc-js/test/test-server.ts index f8b13b30..b30963fb 100644 --- a/packages/grpc-js/test/test-server.ts +++ b/packages/grpc-js/test/test-server.ts @@ -181,7 +181,7 @@ describe('Server', () => { const server = new Server(); assert.throws(() => { - server.addService({} as any, dummyImpls); + server.addService(({} as any), dummyImpls); }, /Cannot add an empty service to a server/); }); From 1fc284f59d67b4120dec5f8fc6372f5aa5741eca Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 2 Sep 2020 15:17:57 -0700 Subject: [PATCH 15/15] Revert "fix(grpc-js): Add support for impl type to server.addService" --- packages/grpc-js/src/server.ts | 10 +++------- packages/grpc-js/test/test-server.ts | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 64c17a02..8d8952e8 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -144,13 +144,9 @@ export class Server { throw new Error('Not implemented. Use addService() instead'); } - addService< - ImplementationType extends { - [key: string]: any - } - >( - service: ServiceDefinition, - implementation: ImplementationType + addService( + service: ServiceDefinition, + implementation: UntypedServiceImplementation ): void { if (this.started === true) { throw new Error("Can't add a service to a started server."); diff --git a/packages/grpc-js/test/test-server.ts b/packages/grpc-js/test/test-server.ts index b30963fb..434efbbc 100644 --- a/packages/grpc-js/test/test-server.ts +++ b/packages/grpc-js/test/test-server.ts @@ -181,7 +181,7 @@ describe('Server', () => { const server = new Server(); assert.throws(() => { - server.addService(({} as any), dummyImpls); + server.addService({}, dummyImpls); }, /Cannot add an empty service to a server/); });