From 49b629ffb0c294b8beed234ce50fc8f21de503bb Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 31 Jul 2023 16:54:26 -0700 Subject: [PATCH 01/38] grpc-js/grpc-js-xds: Update to 1.9.0, and update READMEs --- packages/grpc-js-xds/README.md | 6 ++++-- packages/grpc-js-xds/package.json | 4 ++-- packages/grpc-js/README.md | 1 + packages/grpc-js/package.json | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js-xds/README.md b/packages/grpc-js-xds/README.md index 793e0c0d7..c1db440cf 100644 --- a/packages/grpc-js-xds/README.md +++ b/packages/grpc-js-xds/README.md @@ -1,6 +1,6 @@ # @grpc/grpc-js xDS plugin -This package provides support for the `xds://` URL scheme to the `@grpc/grpc-js` library. The latest version of this package is compatible with `@grpc/grpc-js` version 1.2.x. +This package provides support for the `xds://` URL scheme to the `@grpc/grpc-js` library. The latest version of this package is compatible with `@grpc/grpc-js` version 1.9.x. ## Installation @@ -29,4 +29,6 @@ const client = new MyServiceClient('xds:///example.com:123'); - [xDS Client-Side Fault Injection](https://github.com/grpc/proposal/blob/master/A33-Fault-Injection.md) - [Client Status Discovery Service](https://github.com/grpc/proposal/blob/master/A40-csds-support.md) - [Outlier Detection](https://github.com/grpc/proposal/blob/master/A50-xds-outlier-detection.md) - - [xDS Retry Support](https://github.com/grpc/proposal/blob/master/A44-xds-retry.md) \ No newline at end of file + - [xDS Retry Support](https://github.com/grpc/proposal/blob/master/A44-xds-retry.md) + - [xDS Aggregate and Logical DNS Clusters](https://github.com/grpc/proposal/blob/master/A37-xds-aggregate-and-logical-dns-clusters.md)' + - [xDS Federation](https://github.com/grpc/proposal/blob/master/A47-xds-federation.md) (Currently experimental, enabled by environment variable `GRPC_EXPERIMENTAL_XDS_FEDERATION`) diff --git a/packages/grpc-js-xds/package.json b/packages/grpc-js-xds/package.json index 7fd7e700d..a55c631cf 100644 --- a/packages/grpc-js-xds/package.json +++ b/packages/grpc-js-xds/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js-xds", - "version": "1.8.2", + "version": "1.9.0", "description": "Plugin for @grpc/grpc-js. Adds the xds:// URL scheme and associated features.", "main": "build/src/index.js", "scripts": { @@ -49,7 +49,7 @@ "vscode-uri": "^3.0.7" }, "peerDependencies": { - "@grpc/grpc-js": "~1.8.0" + "@grpc/grpc-js": "~1.9.0" }, "engines": { "node": ">=10.10.0" diff --git a/packages/grpc-js/README.md b/packages/grpc-js/README.md index 112b99932..eb04ece2f 100644 --- a/packages/grpc-js/README.md +++ b/packages/grpc-js/README.md @@ -65,6 +65,7 @@ Many channel arguments supported in `grpc` are not supported in `@grpc/grpc-js`. - `grpc.service_config_disable_resolution` - `grpc.client_idle_timeout_ms` - `grpc-node.max_session_memory` + - `grpc-node.tls_enable_trace` - `channelOverride` - `channelFactoryOverride` diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index f9f5629d5..97ff965a2 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.8.21", + "version": "1.9.0", "description": "gRPC Library for Node - pure JS implementation", "homepage": "https://grpc.io/", "repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js", From bb2942197efa2698b37d8ab688578f9b6c4e5dde Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 2 Aug 2023 16:42:29 -0700 Subject: [PATCH 02/38] grpc-js: Improve formatting of channelz logs for grpcdebug --- packages/grpc-js/src/internal-channel.ts | 4 +--- packages/grpc-js/src/subchannel.ts | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/grpc-js/src/internal-channel.ts b/packages/grpc-js/src/internal-channel.ts index 88dd34741..f183729e8 100644 --- a/packages/grpc-js/src/internal-channel.ts +++ b/packages/grpc-js/src/internal-channel.ts @@ -477,9 +477,7 @@ export class InternalChannel { if (this.channelzEnabled) { this.channelzTrace.addTrace( 'CT_INFO', - ConnectivityState[this.connectivityState] + - ' -> ' + - ConnectivityState[newState] + 'Connectivity state change to ' + ConnectivityState[newState] ); } this.connectivityState = newState; diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index 6fad9500a..91455f7c1 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -277,9 +277,7 @@ export class Subchannel { if (this.channelzEnabled) { this.channelzTrace.addTrace( 'CT_INFO', - ConnectivityState[this.connectivityState] + - ' -> ' + - ConnectivityState[newState] + 'Connectivity state change to ' + ConnectivityState[newState] ); } const previousState = this.connectivityState; From 30bc44f4ce54811022b84e0623d3757bea71736b Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 2 Aug 2023 16:48:57 -0700 Subject: [PATCH 03/38] grpc-js: Handle race between call cancellation and auth metadata generation --- packages/grpc-js/src/load-balancing-call.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/grpc-js/src/load-balancing-call.ts b/packages/grpc-js/src/load-balancing-call.ts index 6e9718a7a..b6c990946 100644 --- a/packages/grpc-js/src/load-balancing-call.ts +++ b/packages/grpc-js/src/load-balancing-call.ts @@ -140,6 +140,12 @@ export class LoadBalancingCall implements Call { .generateMetadata({ service_url: this.serviceUrl }) .then( credsMetadata => { + /* If this call was cancelled (e.g. by the deadline) before + * metadata generation finished, we shouldn't do anything with + * it. */ + if (this.ended) { + return; + } const finalMetadata = this.metadata!.clone(); finalMetadata.merge(credsMetadata); if (finalMetadata.get('authorization').length > 1) { From 01749a8d4199180eab3feda9976993fa642afd8f Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 3 Aug 2023 09:24:24 -0700 Subject: [PATCH 04/38] Explicitly log credentials/cancellation races --- packages/grpc-js/src/load-balancing-call.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/grpc-js/src/load-balancing-call.ts b/packages/grpc-js/src/load-balancing-call.ts index b6c990946..2721e96a4 100644 --- a/packages/grpc-js/src/load-balancing-call.ts +++ b/packages/grpc-js/src/load-balancing-call.ts @@ -144,6 +144,7 @@ export class LoadBalancingCall implements Call { * metadata generation finished, we shouldn't do anything with * it. */ if (this.ended) { + this.trace('Credentials metadata generation finished after call ended'); return; } const finalMetadata = this.metadata!.clone(); From a4ba9253523755c979c53b5cab978f7afd2ba8a6 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 8 Aug 2023 10:37:20 -0700 Subject: [PATCH 05/38] grpc-js: Add null check in pick_first array access --- packages/grpc-js/src/load-balancer-pick-first.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 08971980b..37bc8e0ff 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -306,7 +306,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { this.children[subchannelIndex].subchannel.getAddress() ); process.nextTick(() => { - this.children[subchannelIndex].subchannel.startConnecting(); + this.children[subchannelIndex]?.subchannel.startConnecting(); }); } this.connectionDelayTimeout = setTimeout(() => { From 12217720527d702ca4d263b41b60d17438e08c16 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 14 Aug 2023 10:15:46 -0700 Subject: [PATCH 06/38] grpc-js: Switch Timer type to Timeout --- packages/grpc-js/src/backoff-timeout.ts | 2 +- packages/grpc-js/src/internal-channel.ts | 4 ++-- packages/grpc-js/src/load-balancer-outlier-detection.ts | 2 +- packages/grpc-js/src/resolver-dns.ts | 2 +- packages/grpc-js/src/resolving-call.ts | 2 +- packages/grpc-js/src/retrying-call.ts | 2 +- packages/grpc-js/src/server-call.ts | 2 +- packages/grpc-js/src/server.ts | 6 +++--- packages/grpc-js/src/subchannel-pool.ts | 2 +- packages/grpc-js/src/transport.ts | 4 ++-- 10 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/grpc-js/src/backoff-timeout.ts b/packages/grpc-js/src/backoff-timeout.ts index f523e259a..3ffd26064 100644 --- a/packages/grpc-js/src/backoff-timeout.ts +++ b/packages/grpc-js/src/backoff-timeout.ts @@ -63,7 +63,7 @@ export class BackoffTimeout { * to an object representing a timer that has ended, but it can still be * interacted with without error. */ - private timerId: NodeJS.Timer; + private timerId: NodeJS.Timeout; /** * Indicates whether the timer is currently running. */ diff --git a/packages/grpc-js/src/internal-channel.ts b/packages/grpc-js/src/internal-channel.ts index f183729e8..0ed189c03 100644 --- a/packages/grpc-js/src/internal-channel.ts +++ b/packages/grpc-js/src/internal-channel.ts @@ -166,7 +166,7 @@ export class InternalChannel { * the invariant is that callRefTimer is reffed if and only if pickQueue * is non-empty. */ - private readonly callRefTimer: NodeJS.Timer; + private readonly callRefTimer: NodeJS.Timeout; private configSelector: ConfigSelector | null = null; /** * This is the error from the name resolver if it failed most recently. It @@ -182,7 +182,7 @@ export class InternalChannel { new Set(); private callCount = 0; - private idleTimer: NodeJS.Timer | null = null; + private idleTimer: NodeJS.Timeout | null = null; private readonly idleTimeoutMs: number; // Channelz info diff --git a/packages/grpc-js/src/load-balancer-outlier-detection.ts b/packages/grpc-js/src/load-balancer-outlier-detection.ts index 4abbd0843..3e4b46feb 100644 --- a/packages/grpc-js/src/load-balancer-outlier-detection.ts +++ b/packages/grpc-js/src/load-balancer-outlier-detection.ts @@ -502,7 +502,7 @@ export class OutlierDetectionLoadBalancer implements LoadBalancer { private childBalancer: ChildLoadBalancerHandler; private addressMap: Map = new Map(); private latestConfig: OutlierDetectionLoadBalancingConfig | null = null; - private ejectionTimer: NodeJS.Timer; + private ejectionTimer: NodeJS.Timeout; private timerStartTime: Date | null = null; constructor(channelControlHelper: ChannelControlHelper) { diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index b55278525..c40cb8ec5 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -96,7 +96,7 @@ class DnsResolver implements Resolver { private defaultResolutionError: StatusObject; private backoff: BackoffTimeout; private continueResolving = false; - private nextResolutionTimer: NodeJS.Timer; + private nextResolutionTimer: NodeJS.Timeout; private isNextResolutionTimerRunning = false; private isServiceConfigEnabled = true; constructor( diff --git a/packages/grpc-js/src/resolving-call.ts b/packages/grpc-js/src/resolving-call.ts index 8aa717c06..723533dba 100644 --- a/packages/grpc-js/src/resolving-call.ts +++ b/packages/grpc-js/src/resolving-call.ts @@ -53,7 +53,7 @@ export class ResolvingCall implements Call { private deadline: Deadline; private host: string; private statusWatchers: ((status: StatusObject) => void)[] = []; - private deadlineTimer: NodeJS.Timer = setTimeout(() => {}, 0); + private deadlineTimer: NodeJS.Timeout = setTimeout(() => {}, 0); private filterStack: FilterStack | null = null; constructor( diff --git a/packages/grpc-js/src/retrying-call.ts b/packages/grpc-js/src/retrying-call.ts index c329161c3..e6e1cbb44 100644 --- a/packages/grpc-js/src/retrying-call.ts +++ b/packages/grpc-js/src/retrying-call.ts @@ -194,7 +194,7 @@ export class RetryingCall implements Call { * Number of attempts so far */ private attempts = 0; - private hedgingTimer: NodeJS.Timer | null = null; + private hedgingTimer: NodeJS.Timeout | null = null; private committedCallIndex: number | null = null; private initialRetryBackoffSec = 0; private nextRetryBackoffSec = 0; diff --git a/packages/grpc-js/src/server-call.ts b/packages/grpc-js/src/server-call.ts index b1898fd26..95f928350 100644 --- a/packages/grpc-js/src/server-call.ts +++ b/packages/grpc-js/src/server-call.ts @@ -408,7 +408,7 @@ export class Http2ServerCallStream< ResponseType > extends EventEmitter { cancelled = false; - deadlineTimer: NodeJS.Timer | null = null; + deadlineTimer: NodeJS.Timeout | null = null; private statusSent = false; private deadline: Deadline = Infinity; private wantTrailers = false; diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index f5e79a339..c9308ca62 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -1079,8 +1079,8 @@ export class Server { ); this.sessionChildrenTracker.refChild(channelzRef); } - let connectionAgeTimer: NodeJS.Timer | null = null; - let connectionAgeGraceTimer: NodeJS.Timer | null = null; + let connectionAgeTimer: NodeJS.Timeout | null = null; + let connectionAgeGraceTimer: NodeJS.Timeout | null = null; let sessionClosedByServer = false; if (this.maxConnectionAgeMs !== UNLIMITED_CONNECTION_AGE_MS) { // Apply a random jitter within a +/-10% range @@ -1115,7 +1115,7 @@ export class Server { } }, this.maxConnectionAgeMs + jitter).unref?.(); } - const keeapliveTimeTimer: NodeJS.Timer | null = setInterval(() => { + const keeapliveTimeTimer: NodeJS.Timeout | null = setInterval(() => { const timeoutTImer = setTimeout(() => { sessionClosedByServer = true; if (this.channelzEnabled) { diff --git a/packages/grpc-js/src/subchannel-pool.ts b/packages/grpc-js/src/subchannel-pool.ts index 0cbc028ed..a5dec729d 100644 --- a/packages/grpc-js/src/subchannel-pool.ts +++ b/packages/grpc-js/src/subchannel-pool.ts @@ -45,7 +45,7 @@ export class SubchannelPool { /** * A timer of a task performing a periodic subchannel cleanup. */ - private cleanupTimer: NodeJS.Timer | null = null; + private cleanupTimer: NodeJS.Timeout | null = null; /** * A pool of subchannels use for making connections. Subchannels with the diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 37854e68a..18d83cbfe 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -108,7 +108,7 @@ class Http2Transport implements Transport { /** * Timer reference for timeout that indicates when to send the next ping */ - private keepaliveTimerId: NodeJS.Timer | null = null; + private keepaliveTimerId: NodeJS.Timeout | null = null; /** * Indicates that the keepalive timer ran out while there were no active * calls, and a ping should be sent the next time a call starts. @@ -117,7 +117,7 @@ class Http2Transport implements Transport { /** * Timer reference tracking when the most recent ping will be considered lost */ - private keepaliveTimeoutId: NodeJS.Timer | null = null; + private keepaliveTimeoutId: NodeJS.Timeout | null = null; /** * Indicates whether keepalive pings should be sent without any active calls */ From 69257a78937d60bfe27574c3e10cdd24df0ecc48 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 21 Aug 2023 11:14:17 -0700 Subject: [PATCH 07/38] grpc-js: Fix method config name handling in service configs --- packages/grpc-js/package.json | 2 +- .../grpc-js/src/resolving-load-balancer.ts | 92 ++++++++++++++++--- packages/grpc-js/src/service-config.ts | 40 +++++--- 3 files changed, 107 insertions(+), 27 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 97ff965a2..800dde9ac 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.0", + "version": "1.9.1", "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/resolving-load-balancer.ts b/packages/grpc-js/src/resolving-load-balancer.ts index d49609ff2..9b5d4c2dc 100644 --- a/packages/grpc-js/src/resolving-load-balancer.ts +++ b/packages/grpc-js/src/resolving-load-balancer.ts @@ -21,7 +21,11 @@ import { LoadBalancingConfig, getFirstUsableConfig, } from './load-balancer'; -import { ServiceConfig, validateServiceConfig } from './service-config'; +import { + MethodConfig, + ServiceConfig, + validateServiceConfig, +} from './service-config'; import { ConnectivityState } from './connectivity-state'; import { ConfigSelector, createResolver, Resolver } from './resolver'; import { ServiceError } from './call'; @@ -43,6 +47,59 @@ function trace(text: string): void { logging.trace(LogVerbosity.DEBUG, TRACER_NAME, text); } +type NameMatchLevel = 'EMPTY' | 'SERVICE' | 'SERVICE_AND_METHOD'; + +/** + * Name match levels in order from most to least specific. This is the order in + * which searches will be performed. + */ +const NAME_MATCH_LEVEL_ORDER: NameMatchLevel[] = [ + 'SERVICE_AND_METHOD', + 'SERVICE', + 'EMPTY', +]; + +function hasMatchingName( + service: string, + method: string, + methodConfig: MethodConfig, + matchLevel: NameMatchLevel +): boolean { + for (const name of methodConfig.name) { + switch (matchLevel) { + case 'EMPTY': + if (!name.service && !name.method) { + return true; + } + break; + case 'SERVICE': + if (name.service === service && !name.method) { + return true; + } + break; + case 'SERVICE_AND_METHOD': + if (name.service === service && name.method === method) { + return true; + } + } + } + return false; +} + +function findMatchingConfig( + service: string, + method: string, + methodConfigs: MethodConfig[], + matchLevel: NameMatchLevel +): MethodConfig | null { + for (const config of methodConfigs) { + if (hasMatchingName(service, method, config, matchLevel)) { + return config; + } + } + return null; +} + function getDefaultConfigSelector( serviceConfig: ServiceConfig | null ): ConfigSelector { @@ -54,19 +111,26 @@ function getDefaultConfigSelector( const service = splitName[0] ?? ''; const method = splitName[1] ?? ''; if (serviceConfig && serviceConfig.methodConfig) { - for (const methodConfig of serviceConfig.methodConfig) { - for (const name of methodConfig.name) { - if ( - name.service === service && - (name.method === undefined || name.method === method) - ) { - return { - methodConfig: methodConfig, - pickInformation: {}, - status: Status.OK, - dynamicFilterFactories: [], - }; - } + /* Check for the following in order, and return the first method + * config that matches: + * 1. A name that exactly matches the service and method + * 2. A name with no method set that matches the service + * 3. An empty name + */ + for (const matchLevel of NAME_MATCH_LEVEL_ORDER) { + const matchingConfig = findMatchingConfig( + service, + method, + serviceConfig.methodConfig, + matchLevel + ); + if (matchingConfig) { + return { + methodConfig: matchingConfig, + pickInformation: {}, + status: Status.OK, + dynamicFilterFactories: [], + }; } } } diff --git a/packages/grpc-js/src/service-config.ts b/packages/grpc-js/src/service-config.ts index 91bee52c2..aece7cb77 100644 --- a/packages/grpc-js/src/service-config.ts +++ b/packages/grpc-js/src/service-config.ts @@ -35,7 +35,7 @@ import { } from './load-balancer'; export interface MethodConfigName { - service: string; + service?: string; method?: string; } @@ -95,20 +95,36 @@ const DURATION_REGEX = /^\d+(\.\d{1,9})?s$/; const CLIENT_LANGUAGE_STRING = 'node'; function validateName(obj: any): MethodConfigName { - if (!('service' in obj) || typeof obj.service !== 'string') { - throw new Error('Invalid method config name: invalid service'); - } - const result: MethodConfigName = { - service: obj.service, - }; - if ('method' in obj) { - if (typeof obj.method === 'string') { - result.method = obj.method; + // In this context, and unset field and '' are considered the same + if ('service' in obj && obj.service !== '') { + if (typeof obj.service !== 'string') { + throw new Error( + `Invalid method config name: invalid service: expected type string, got ${typeof obj.service}` + ); + } + if ('method' in obj && obj.method !== '') { + if (typeof obj.method !== 'string') { + throw new Error( + `Invalid method config name: invalid method: expected type string, got ${typeof obj.service}` + ); + } + return { + service: obj.service, + method: obj.method, + }; } else { - throw new Error('Invalid method config name: invalid method'); + return { + service: obj.service, + }; } + } else { + if ('method' in obj && obj.method !== undefined) { + throw new Error( + `Invalid method config name: method set with empty or unset service` + ); + } + return {}; } - return result; } function validateRetryPolicy(obj: any): RetryPolicy { From f9af919393a6753ad7cb144aa4695d63809fc6e1 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 21 Aug 2023 13:17:11 -0700 Subject: [PATCH 08/38] grpc-js: Update dependency on @grpc/proto-loader --- packages/grpc-js/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 97ff965a2..70fd573ca 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -65,7 +65,7 @@ "generate-test-types": "proto-loader-gen-types --keepCase --longs String --enums String --defaults --oneofs --includeComments --include-dirs test/fixtures/ -O test/generated/ --grpcLib ../../src/index test_service.proto" }, "dependencies": { - "@grpc/proto-loader": "^0.7.0", + "@grpc/proto-loader": "^0.7.8", "@types/node": ">=12.12.47" }, "files": [ From 8896bfe4c969b756b3e18d2e83ec4480aeac3a9e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 21 Aug 2023 13:30:33 -0700 Subject: [PATCH 09/38] grpc-js: Defer actions in http2 stream write callback --- packages/grpc-js/src/subchannel-call.ts | 26 +++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/grpc-js/src/subchannel-call.ts b/packages/grpc-js/src/subchannel-call.ts index e06ece388..3b9b6152f 100644 --- a/packages/grpc-js/src/subchannel-call.ts +++ b/packages/grpc-js/src/subchannel-call.ts @@ -501,16 +501,22 @@ export class Http2SubchannelCall implements SubchannelCall { sendMessageWithContext(context: MessageContext, message: Buffer) { this.trace('write() called with message of length ' + message.length); const cb: WriteCallback = (error?: Error | null) => { - let code: Status = Status.UNAVAILABLE; - if ( - (error as NodeJS.ErrnoException)?.code === 'ERR_STREAM_WRITE_AFTER_END' - ) { - code = Status.INTERNAL; - } - if (error) { - this.cancelWithStatus(code, `Write error: ${error.message}`); - } - context.callback?.(); + /* nextTick here ensures that no stream action can be taken in the call + * stack of the write callback, in order to hopefully work around + * https://github.com/nodejs/node/issues/49147 */ + process.nextTick(() => { + let code: Status = Status.UNAVAILABLE; + if ( + (error as NodeJS.ErrnoException)?.code === + 'ERR_STREAM_WRITE_AFTER_END' + ) { + code = Status.INTERNAL; + } + if (error) { + this.cancelWithStatus(code, `Write error: ${error.message}`); + } + context.callback?.(); + }); }; this.trace('sending data chunk of length ' + message.length); this.callEventTracker.addMessageSent(); From a0e028f788c0845aa62dc1f657d80ba06541c333 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 22 Aug 2023 11:19:23 -0700 Subject: [PATCH 10/38] grpc-js-xds: Fix backoff timer reference when handling LRS stream messages --- packages/grpc-js-xds/src/xds-client.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js-xds/src/xds-client.ts b/packages/grpc-js-xds/src/xds-client.ts index 2ae9618b3..020f767b5 100644 --- a/packages/grpc-js-xds/src/xds-client.ts +++ b/packages/grpc-js-xds/src/xds-client.ts @@ -602,9 +602,9 @@ class ClusterLoadReportMap { * Get the indicated map entry if it exists, or create a new one if it does * not. Increments the refcount of that entry, so a call to this method * should correspond to a later call to unref - * @param clusterName - * @param edsServiceName - * @returns + * @param clusterName + * @param edsServiceName + * @returns */ getOrCreate(clusterName: string, edsServiceName: string): ClusterLoadReport { for (const statsObj of this.statsMap) { @@ -924,8 +924,8 @@ class XdsSingleServerClient { } onLrsStreamReceivedMessage() { - this.adsBackoff.stop(); - this.adsBackoff.reset(); + this.lrsBackoff.stop(); + this.lrsBackoff.reset(); } handleLrsStreamEnd() { From 83789c15dbe9de3bc9069bc0d7c63f13d71f5b6e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 31 Aug 2023 09:30:26 -0700 Subject: [PATCH 11/38] grpc-js: Handle keepalive ping error --- packages/grpc-js/src/transport.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 18d83cbfe..49ec01ddc 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -426,6 +426,10 @@ class Http2Transport implements Transport { try { this.session!.ping( (err: Error | null, duration: number, payload: Buffer) => { + if (err) { + this.keepaliveTrace('Ping failed with error ' + err.message); + this.handleDisconnect(); + } this.keepaliveTrace('Received ping response'); this.clearKeepaliveTimeout(); this.maybeStartKeepalivePingTimer(); From f5218edf820802c5649107c018d27796b438055f Mon Sep 17 00:00:00 2001 From: gusumuzhe Date: Tue, 29 Aug 2023 17:39:38 +0800 Subject: [PATCH 12/38] fix: pick first load balancer call doPick infinite --- packages/grpc-js/src/load-balancer-pick-first.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 37bc8e0ff..0f2d7d654 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -315,7 +315,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { } private pickSubchannel(subchannel: SubchannelInterface) { - if (subchannel === this.currentPick) { + if (this.currentPick && subchannel.realSubchannelEquals(this.currentPick)) { return; } trace('Pick subchannel with address ' + subchannel.getAddress()); From 2fe961d5b10017183c231bc6161e768e6551a74a Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 31 Aug 2023 09:37:34 -0700 Subject: [PATCH 13/38] grpc-js: Bump to version 1.9.2 --- packages/grpc-js/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 196ce8f4a..974f196c4 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.1", + "version": "1.9.2", "description": "gRPC Library for Node - pure JS implementation", "homepage": "https://grpc.io/", "repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js", From f1f8d1ba619a4756133699854426b6aea386a744 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 11 Sep 2023 13:51:32 -0700 Subject: [PATCH 14/38] grpc-js: Make a few improvements to DNS resolving timing --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/resolver-dns.ts | 3 ++- packages/grpc-js/src/resolving-load-balancer.ts | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 974f196c4..7b8801212 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.2", + "version": "1.9.3", "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 c40cb8ec5..6bb31a3a1 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -175,6 +175,7 @@ class DnsResolver implements Resolver { }); this.backoff.stop(); this.backoff.reset(); + this.stopNextResolutionTimer(); return; } if (this.dnsHostname === null) { @@ -339,9 +340,9 @@ class DnsResolver implements Resolver { private startResolutionWithBackoff() { if (this.pendingLookupPromise === null) { this.continueResolving = false; - this.startResolution(); this.backoff.runOnce(); this.startNextResolutionTimer(); + this.startResolution(); } } diff --git a/packages/grpc-js/src/resolving-load-balancer.ts b/packages/grpc-js/src/resolving-load-balancer.ts index 9b5d4c2dc..425d1fe2e 100644 --- a/packages/grpc-js/src/resolving-load-balancer.ts +++ b/packages/grpc-js/src/resolving-load-balancer.ts @@ -247,6 +247,8 @@ export class ResolvingLoadBalancer implements LoadBalancer { configSelector: ConfigSelector | null, attributes: { [key: string]: unknown } ) => { + this.backoffTimeout.stop(); + this.backoffTimeout.reset(); let workingServiceConfig: ServiceConfig | null = null; /* This first group of conditionals implements the algorithm described * in https://github.com/grpc/proposal/blob/master/A21-service-config-error-handling.md From 10c4bbdbe391bb806d55fe4070af811c79174834 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 13 Sep 2023 10:18:30 -0700 Subject: [PATCH 15/38] Add logging for DNS update delays due to rate limit or backoff --- packages/grpc-js/src/backoff-timeout.ts | 15 +++++++++++++++ packages/grpc-js/src/resolver-dns.ts | 5 +++++ packages/grpc-js/src/resolving-load-balancer.ts | 1 + 3 files changed, 21 insertions(+) diff --git a/packages/grpc-js/src/backoff-timeout.ts b/packages/grpc-js/src/backoff-timeout.ts index 3ffd26064..78318d1e8 100644 --- a/packages/grpc-js/src/backoff-timeout.ts +++ b/packages/grpc-js/src/backoff-timeout.ts @@ -78,6 +78,11 @@ export class BackoffTimeout { * running is true. */ private startTime: Date = new Date(); + /** + * The approximate time that the currently running timer will end. Only valid + * if running is true. + */ + private endTime: Date = new Date(); constructor(private callback: () => void, options?: BackoffOptions) { if (options) { @@ -100,6 +105,8 @@ export class BackoffTimeout { } private runTimer(delay: number) { + this.endTime = this.startTime; + this.endTime.setMilliseconds(this.endTime.getMilliseconds() + this.nextDelay); clearTimeout(this.timerId); this.timerId = setTimeout(() => { this.callback(); @@ -178,4 +185,12 @@ export class BackoffTimeout { this.hasRef = false; this.timerId.unref?.(); } + + /** + * Get the approximate timestamp of when the timer will fire. Only valid if + * this.isRunning() is true. + */ + getEndTime() { + return this.endTime; + } } diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 6bb31a3a1..149530bbd 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -353,6 +353,11 @@ class DnsResolver implements Resolver { * fires. Otherwise, start resolving immediately. */ if (this.pendingLookupPromise === null) { if (this.isNextResolutionTimerRunning || this.backoff.isRunning()) { + if (this.isNextResolutionTimerRunning) { + trace('resolution update delayed by "min time between resolutions" rate limit'); + } else { + trace('resolution update delayed by backoff timer until ' + this.backoff.getEndTime().toISOString()); + } this.continueResolving = true; } else { this.startResolutionWithBackoff(); diff --git a/packages/grpc-js/src/resolving-load-balancer.ts b/packages/grpc-js/src/resolving-load-balancer.ts index 425d1fe2e..ceec4b5d1 100644 --- a/packages/grpc-js/src/resolving-load-balancer.ts +++ b/packages/grpc-js/src/resolving-load-balancer.ts @@ -222,6 +222,7 @@ export class ResolvingLoadBalancer implements LoadBalancer { * In that case, the backoff timer callback will call * updateResolution */ if (this.backoffTimeout.isRunning()) { + trace('requestReresolution delayed by backoff timer until ' + this.backoffTimeout.getEndTime().toISOString()); this.continueResolving = true; } else { this.updateResolution(); From c8b9a45bc952a3586535d909088609552f54ab8e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 21 Sep 2023 10:01:38 -0700 Subject: [PATCH 16/38] grpc-js-xds: Fix behavior when channel goes IDLE --- packages/grpc-js-xds/package.json | 2 +- packages/grpc-js-xds/src/resolver-xds.ts | 14 ++++++----- packages/grpc-js-xds/test/client.ts | 14 +++++++---- packages/grpc-js-xds/test/test-core.ts | 31 ++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/packages/grpc-js-xds/package.json b/packages/grpc-js-xds/package.json index a55c631cf..606bd9faf 100644 --- a/packages/grpc-js-xds/package.json +++ b/packages/grpc-js-xds/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js-xds", - "version": "1.9.0", + "version": "1.9.1", "description": "Plugin for @grpc/grpc-js. Adds the xds:// URL scheme and associated features.", "main": "build/src/index.js", "scripts": { diff --git a/packages/grpc-js-xds/src/resolver-xds.ts b/packages/grpc-js-xds/src/resolver-xds.ts index a934e7285..17d3c6305 100644 --- a/packages/grpc-js-xds/src/resolver-xds.ts +++ b/packages/grpc-js-xds/src/resolver-xds.ts @@ -213,7 +213,7 @@ function getPredicateForMatcher(routeMatch: RouteMatch__Output): Matcher { * the ServiceConfig definition. The difference is that the protobuf message * defines seconds as a long, which is represented as a string in JavaScript, * and the one used in the service config defines it as a number. - * @param duration + * @param duration */ function protoDurationToDuration(duration: Duration__Output): Duration { return { @@ -235,7 +235,7 @@ function getDefaultRetryMaxInterval(baseInterval: string): string { /** * Encode a text string as a valid path of a URI, as specified in RFC-3986 section 3.3 * @param uriPath A value representing an unencoded URI path - * @returns + * @returns */ function encodeURIPath(uriPath: string): string { return uriPath.replace(/[^A-Za-z0-9._~!$&^()*+,;=/-]/g, substring => encodeURIComponent(substring)); @@ -447,7 +447,7 @@ class XdsResolver implements Resolver { } } } - let retryPolicy: RetryPolicy | undefined = undefined; + let retryPolicy: RetryPolicy | undefined = undefined; if (EXPERIMENTAL_RETRY) { const retryConfig = route.route!.retry_policy ?? virtualHost.retry_policy; if (retryConfig) { @@ -458,10 +458,10 @@ class XdsResolver implements Resolver { } } if (retryableStatusCodes.length > 0) { - const baseInterval = retryConfig.retry_back_off?.base_interval ? - protoDurationToSecondsString(retryConfig.retry_back_off.base_interval) : + const baseInterval = retryConfig.retry_back_off?.base_interval ? + protoDurationToSecondsString(retryConfig.retry_back_off.base_interval) : DEFAULT_RETRY_BASE_INTERVAL; - const maxInterval = retryConfig.retry_back_off?.max_interval ? + const maxInterval = retryConfig.retry_back_off?.max_interval ? protoDurationToSecondsString(retryConfig.retry_back_off.max_interval) : getDefaultRetryMaxInterval(baseInterval); retryPolicy = { @@ -664,9 +664,11 @@ class XdsResolver implements Resolver { destroy() { if (this.listenerResourceName) { ListenerResourceType.cancelWatch(this.xdsClient, this.listenerResourceName, this.ldsWatcher); + this.isLdsWatcherActive = false; } if (this.latestRouteConfigName) { RouteConfigurationResourceType.cancelWatch(this.xdsClient, this.latestRouteConfigName, this.rdsWatcher); + this.latestRouteConfigName = null; } } diff --git a/packages/grpc-js-xds/test/client.ts b/packages/grpc-js-xds/test/client.ts index 6d346f918..0779702bb 100644 --- a/packages/grpc-js-xds/test/client.ts +++ b/packages/grpc-js-xds/test/client.ts @@ -15,7 +15,7 @@ * */ -import { credentials, loadPackageDefinition, ServiceError } from "@grpc/grpc-js"; +import { ChannelOptions, credentials, loadPackageDefinition, ServiceError } from "@grpc/grpc-js"; import { loadSync } from "@grpc/proto-loader"; import { ProtoGrpcType } from "./generated/echo"; import { EchoTestServiceClient } from "./generated/grpc/testing/EchoTestService"; @@ -44,14 +44,14 @@ export class XdsTestClient { private client: EchoTestServiceClient; private callInterval: NodeJS.Timer; - constructor(target: string, bootstrapInfo: string) { - this.client = new loadedProtos.grpc.testing.EchoTestService(target, credentials.createInsecure(), {[BOOTSTRAP_CONFIG_KEY]: bootstrapInfo}); + constructor(target: string, bootstrapInfo: string, options?: ChannelOptions) { + this.client = new loadedProtos.grpc.testing.EchoTestService(target, credentials.createInsecure(), {...options, [BOOTSTRAP_CONFIG_KEY]: bootstrapInfo}); this.callInterval = setInterval(() => {}, 0); clearInterval(this.callInterval); } - static createFromServer(targetName: string, xdsServer: XdsServer) { - return new XdsTestClient(`xds:///${targetName}`, xdsServer.getBootstrapInfoString()); + static createFromServer(targetName: string, xdsServer: XdsServer, options?: ChannelOptions) { + return new XdsTestClient(`xds:///${targetName}`, xdsServer.getBootstrapInfoString(), options); } startCalls(interval: number) { @@ -98,4 +98,8 @@ export class XdsTestClient { } sendInner(count, callback); } + + getConnectivityState() { + return this.client.getChannel().getConnectivityState(false); + } } diff --git a/packages/grpc-js-xds/test/test-core.ts b/packages/grpc-js-xds/test/test-core.ts index cb145eb81..f48ab6c11 100644 --- a/packages/grpc-js-xds/test/test-core.ts +++ b/packages/grpc-js-xds/test/test-core.ts @@ -22,6 +22,7 @@ import { XdsServer } from "./xds-server"; import { register } from "../src"; import assert = require("assert"); +import { connectivityState } from "@grpc/grpc-js"; register(); @@ -60,4 +61,34 @@ describe('core xDS functionality', () => { }, reason => done(reason)); }, reason => done(reason)); }); + it('should be able to enter and exit idle', function(done) { + this.timeout(5000); + const cluster = new FakeEdsCluster('cluster1', 'endpoint1', [{backends: [new Backend()], locality:{region: 'region1'}}]); + const routeGroup = new FakeRouteGroup('listener1', 'route1', [{cluster: cluster}]); + routeGroup.startAllBackends().then(() => { + xdsServer.setEdsResource(cluster.getEndpointConfig()); + xdsServer.setCdsResource(cluster.getClusterConfig()); + xdsServer.setRdsResource(routeGroup.getRouteConfiguration()); + xdsServer.setLdsResource(routeGroup.getListener()); + xdsServer.addResponseListener((typeUrl, responseState) => { + if (responseState.state === 'NACKED') { + client.stopCalls(); + assert.fail(`Client NACKED ${typeUrl} resource with message ${responseState.errorMessage}`); + } + }) + client = XdsTestClient.createFromServer('listener1', xdsServer, { + 'grpc.client_idle_timeout_ms': 1000, + }); + client.sendOneCall(error => { + assert.ifError(error); + assert.strictEqual(client.getConnectivityState(), connectivityState.READY); + setTimeout(() => { + assert.strictEqual(client.getConnectivityState(), connectivityState.IDLE); + client.sendOneCall(error => { + done(error); + }) + }, 1100); + }); + }, reason => done(reason)); + }); }); From e1415fe7bc86a625d4b95f82728dccb0ca809b65 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 25 Sep 2023 10:24:28 -0700 Subject: [PATCH 17/38] grpc-js-xds: Force submodule update and code generation in prepare script --- packages/grpc-js-xds/package.json | 4 ++-- packages/grpc-js-xds/src/generated/cluster.ts | 9 --------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/grpc-js-xds/package.json b/packages/grpc-js-xds/package.json index 606bd9faf..ccf5c1d0c 100644 --- a/packages/grpc-js-xds/package.json +++ b/packages/grpc-js-xds/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js-xds", - "version": "1.9.1", + "version": "1.9.2", "description": "Plugin for @grpc/grpc-js. Adds the xds:// URL scheme and associated features.", "main": "build/src/index.js", "scripts": { @@ -9,7 +9,7 @@ "clean": "gts clean", "compile": "tsc", "fix": "gts fix", - "prepare": "npm run compile", + "prepare": "git submodule update --init --recursive && npm run generate-types && npm run compile", "pretest": "npm run compile", "posttest": "npm run check", "generate-types": "proto-loader-gen-types --keepCase --longs String --enums String --defaults --oneofs --includeComments --includeDirs deps/envoy-api/ deps/xds/ deps/googleapis/ deps/protoc-gen-validate/ -O src/generated/ --grpcLib @grpc/grpc-js envoy/service/discovery/v3/ads.proto envoy/service/load_stats/v3/lrs.proto envoy/config/listener/v3/listener.proto envoy/config/route/v3/route.proto envoy/config/cluster/v3/cluster.proto envoy/config/endpoint/v3/endpoint.proto envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto udpa/type/v1/typed_struct.proto xds/type/v3/typed_struct.proto envoy/extensions/filters/http/fault/v3/fault.proto envoy/service/status/v3/csds.proto", diff --git a/packages/grpc-js-xds/src/generated/cluster.ts b/packages/grpc-js-xds/src/generated/cluster.ts index 681bc5a2d..78ac3bbd3 100644 --- a/packages/grpc-js-xds/src/generated/cluster.ts +++ b/packages/grpc-js-xds/src/generated/cluster.ts @@ -97,15 +97,6 @@ export interface ProtoGrpcType { } } } - extensions: { - clusters: { - aggregate: { - v3: { - ClusterConfig: MessageTypeDefinition - } - } - } - } type: { matcher: { v3: { From e6099d71f2764b8abeaf9d1bbf166c73b55bf2fd Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 26 Sep 2023 15:17:55 -0700 Subject: [PATCH 18/38] grpc-js: Unref backoff timer in subchannel --- packages/grpc-js/src/subchannel.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index 91455f7c1..0f8844720 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -120,6 +120,7 @@ export class Subchannel { this.backoffTimeout = new BackoffTimeout(() => { this.handleBackoffTimer(); }, backoffOptions); + this.backoffTimeout.unref(); this.subchannelAddressString = subchannelAddressToString(subchannelAddress); this.keepaliveTime = options['grpc.keepalive_time_ms'] ?? -1; From 4c6869091e33580fa868e5ad12406fd21fe62341 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 26 Sep 2023 16:06:03 -0700 Subject: [PATCH 19/38] grpc-js-xds: Don't call git commands in npm scripts --- packages/grpc-js-xds/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js-xds/package.json b/packages/grpc-js-xds/package.json index ccf5c1d0c..a6f0fcf1d 100644 --- a/packages/grpc-js-xds/package.json +++ b/packages/grpc-js-xds/package.json @@ -9,7 +9,7 @@ "clean": "gts clean", "compile": "tsc", "fix": "gts fix", - "prepare": "git submodule update --init --recursive && npm run generate-types && npm run compile", + "prepare": "npm run generate-types && npm run compile", "pretest": "npm run compile", "posttest": "npm run check", "generate-types": "proto-loader-gen-types --keepCase --longs String --enums String --defaults --oneofs --includeComments --includeDirs deps/envoy-api/ deps/xds/ deps/googleapis/ deps/protoc-gen-validate/ -O src/generated/ --grpcLib @grpc/grpc-js envoy/service/discovery/v3/ads.proto envoy/service/load_stats/v3/lrs.proto envoy/config/listener/v3/listener.proto envoy/config/route/v3/route.proto envoy/config/cluster/v3/cluster.proto envoy/config/endpoint/v3/endpoint.proto envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto udpa/type/v1/typed_struct.proto xds/type/v3/typed_struct.proto envoy/extensions/filters/http/fault/v3/fault.proto envoy/service/status/v3/csds.proto", From ea6ba89ead05b603d42a5075b47e05107e684c02 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 26 Sep 2023 16:35:13 -0700 Subject: [PATCH 20/38] grpc-js: Bump version to 1.9.4 --- packages/grpc-js/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 7b8801212..5dbf72506 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.3", + "version": "1.9.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", From 974b235a04b6df4b070e693a6d2aefa8449f0c98 Mon Sep 17 00:00:00 2001 From: Rafael Santos Date: Fri, 29 Sep 2023 15:44:42 +0100 Subject: [PATCH 21/38] Update server-call.ts Fix TS2345 --- packages/grpc-js/src/server-call.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/server-call.ts b/packages/grpc-js/src/server-call.ts index 95f928350..107c2e3ef 100644 --- a/packages/grpc-js/src/server-call.ts +++ b/packages/grpc-js/src/server-call.ts @@ -558,7 +558,7 @@ export class Http2ServerCallStream< return metadata; } - receiveUnaryMessage(encoding: string): Promise { + receiveUnaryMessage(encoding: string): Promise { return new Promise((resolve, reject) => { const { stream } = this; From b33b8bc2bbbc58430ade1688a3b71a14f29887b7 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 29 Sep 2023 11:17:23 -0700 Subject: [PATCH 22/38] grpc-js: Handle race between bindAsync and (try|force)Shutdown --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/server.ts | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 5dbf72506..003663b6f 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.4", + "version": "1.9.5", "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/server.ts b/packages/grpc-js/src/server.ts index c9308ca62..0d8e0825b 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -161,6 +161,7 @@ export class Server { >(); private sessions = new Map(); private started = false; + private shutdown = false; private options: ChannelOptions; private serverAddressString = 'null'; @@ -375,6 +376,10 @@ export class Server { throw new Error('server is already started'); } + if (this.shutdown) { + throw new Error('bindAsync called after shutdown'); + } + if (typeof port !== 'string') { throw new TypeError('port must be a string'); } @@ -485,6 +490,11 @@ export class Server { http2Server.once('error', onError); http2Server.listen(addr, () => { + if (this.shutdown) { + http2Server.close(); + resolve(new Error('bindAsync failed because server is shutdown')); + return; + } const boundAddress = http2Server.address()!; let boundSubchannelAddress: SubchannelAddress; if (typeof boundAddress === 'string') { @@ -583,6 +593,11 @@ export class Server { http2Server.once('error', onError); http2Server.listen(address, () => { + if (this.shutdown) { + http2Server.close(); + resolve({port: 0, count: 0}); + return; + } const boundAddress = http2Server.address() as AddressInfo; const boundSubchannelAddress: SubchannelAddress = { host: boundAddress.address, @@ -637,6 +652,12 @@ export class Server { ) => { // We only want one resolution result. Discard all future results resolverListener.onSuccessfulResolution = () => {}; + if (this.shutdown) { + deferredCallback( + new Error(`bindAsync failed because server is shutdown`), + 0 + ); + } if (addressList.length === 0) { deferredCallback( new Error(`No addresses resolved for port ${port}`), @@ -707,6 +728,7 @@ export class Server { } this.started = false; + this.shutdown = true; // Always destroy any available sessions. It's possible that one or more // tryShutdown() calls are in progress. Don't wait on them to finish. @@ -785,6 +807,7 @@ export class Server { // Close the server if necessary. this.started = false; + this.shutdown = true; for (const { server: http2Server, channelzRef: ref } of this .http2ServerList) { From 0f8ebbdd1786ad134ea17af2615e6070ee961bc1 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 16 Oct 2023 17:06:32 -0700 Subject: [PATCH 23/38] grpc-js: Include library version and PID in all trace logs --- doc/environment_variables.md | 3 +-- packages/grpc-js/src/index.ts | 7 ------- packages/grpc-js/src/logging.ts | 5 ++++- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/doc/environment_variables.md b/doc/environment_variables.md index 70b32e715..1b5ad26af 100644 --- a/doc/environment_variables.md +++ b/doc/environment_variables.md @@ -42,7 +42,6 @@ can be set. - `subchannel_internals` - Traces HTTP/2 session state. Includes per-call logs. - `channel_stacktrace` - Traces channel construction events with stack traces. - `keepalive` - Traces gRPC keepalive pings - - `index` - Traces module loading - `outlier_detection` - Traces outlier detection events The following tracers are added by the `@grpc/grpc-js-xds` library: @@ -62,4 +61,4 @@ can be set. - DEBUG - log all gRPC messages - INFO - log INFO and ERROR message - ERROR - log only errors (default) - - NONE - won't log any \ No newline at end of file + - NONE - won't log any diff --git a/packages/grpc-js/src/index.ts b/packages/grpc-js/src/index.ts index adacae08f..28fc776c7 100644 --- a/packages/grpc-js/src/index.ts +++ b/packages/grpc-js/src/index.ts @@ -273,14 +273,7 @@ import * as load_balancer_outlier_detection from './load-balancer-outlier-detect import * as channelz from './channelz'; import { Deadline } from './deadline'; -const clientVersion = require('../../package.json').version; - (() => { - logging.trace( - LogVerbosity.DEBUG, - 'index', - 'Loading @grpc/grpc-js version ' + clientVersion - ); resolver_dns.setup(); resolver_uds.setup(); resolver_ip.setup(); diff --git a/packages/grpc-js/src/logging.ts b/packages/grpc-js/src/logging.ts index 83438ef73..e1b396fff 100644 --- a/packages/grpc-js/src/logging.ts +++ b/packages/grpc-js/src/logging.ts @@ -16,6 +16,9 @@ */ import { LogVerbosity } from './constants'; +import { pid } from 'process'; + +const clientVersion = require('../../package.json').version; const DEFAULT_LOGGER: Partial = { error: (message?: any, ...optionalParams: any[]) => { @@ -109,7 +112,7 @@ export function trace( text: string ): void { if (isTracerEnabled(tracer)) { - log(severity, new Date().toISOString() + ' | ' + tracer + ' | ' + text); + log(severity, new Date().toISOString() + ' | v' + clientVersion + ' ' + pid + ' | ' + tracer + ' | ' + text); } } From 3a9f4d2aa698ec595c2b923e6aac6d478d4b2a44 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 16 Oct 2023 16:52:41 -0700 Subject: [PATCH 24/38] grpc-js: Propagate connectivity error information to request errors --- packages/grpc-js/package.json | 2 +- .../grpc-js/src/load-balancer-pick-first.ts | 20 +++++++++++++++---- .../grpc-js/src/load-balancer-round-robin.ts | 12 ++++++++--- packages/grpc-js/src/picker.ts | 17 +++++++--------- packages/grpc-js/src/subchannel-interface.ts | 3 ++- packages/grpc-js/src/subchannel.ts | 8 +++++--- packages/grpc-js/src/transport.ts | 9 +++++++-- 7 files changed, 47 insertions(+), 24 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 003663b6f..9d66b4095 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.5", + "version": "1.9.6", "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/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 0f2d7d654..1a22b3f76 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -153,9 +153,11 @@ export class PickFirstLoadBalancer implements LoadBalancer { private subchannelStateListener: ConnectivityStateListener = ( subchannel, previousState, - newState + newState, + keepaliveTime, + errorMessage ) => { - this.onSubchannelStateUpdate(subchannel, previousState, newState); + this.onSubchannelStateUpdate(subchannel, previousState, newState, errorMessage); }; /** * Timer reference for the timer tracking when to start @@ -172,6 +174,12 @@ export class PickFirstLoadBalancer implements LoadBalancer { */ private stickyTransientFailureMode = false; + /** + * The most recent error reported by any subchannel as it transitioned to + * TRANSIENT_FAILURE. + */ + private lastError: string | null = null; + /** * Load balancer that attempts to connect to each backend in the address list * in order, and picks the first one that connects, using it for every @@ -200,7 +208,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { if (this.stickyTransientFailureMode) { this.updateState( ConnectivityState.TRANSIENT_FAILURE, - new UnavailablePicker() + new UnavailablePicker({details: `No connection established. Last error: ${this.lastError}`}) ); } else { this.updateState(ConnectivityState.CONNECTING, new QueuePicker(this)); @@ -241,7 +249,8 @@ export class PickFirstLoadBalancer implements LoadBalancer { private onSubchannelStateUpdate( subchannel: SubchannelInterface, previousState: ConnectivityState, - newState: ConnectivityState + newState: ConnectivityState, + errorMessage?: string ) { if (this.currentPick?.realSubchannelEquals(subchannel)) { if (newState !== ConnectivityState.READY) { @@ -258,6 +267,9 @@ export class PickFirstLoadBalancer implements LoadBalancer { } if (newState === ConnectivityState.TRANSIENT_FAILURE) { child.hasReportedTransientFailure = true; + if (errorMessage) { + this.lastError = errorMessage; + } this.maybeEnterStickyTransientFailureMode(); if (index === this.currentSubchannelIndex) { this.startNextSubchannelConnecting(index + 1); diff --git a/packages/grpc-js/src/load-balancer-round-robin.ts b/packages/grpc-js/src/load-balancer-round-robin.ts index f389fefc0..062aa9f0d 100644 --- a/packages/grpc-js/src/load-balancer-round-robin.ts +++ b/packages/grpc-js/src/load-balancer-round-robin.ts @@ -105,18 +105,24 @@ export class RoundRobinLoadBalancer implements LoadBalancer { private currentReadyPicker: RoundRobinPicker | null = null; + private lastError: string | null = null; + constructor(private readonly channelControlHelper: ChannelControlHelper) { this.subchannelStateListener = ( subchannel: SubchannelInterface, previousState: ConnectivityState, - newState: ConnectivityState + newState: ConnectivityState, + keepaliveTime: number, + errorMessage?: string ) => { this.calculateAndUpdateState(); - if ( newState === ConnectivityState.TRANSIENT_FAILURE || newState === ConnectivityState.IDLE ) { + if (errorMessage) { + this.lastError = errorMessage; + } this.channelControlHelper.requestReresolution(); subchannel.startConnecting(); } @@ -157,7 +163,7 @@ export class RoundRobinLoadBalancer implements LoadBalancer { ) { this.updateState( ConnectivityState.TRANSIENT_FAILURE, - new UnavailablePicker() + new UnavailablePicker({details: `No connection established. Last error: ${this.lastError}`}) ); } else { this.updateState(ConnectivityState.IDLE, new QueuePicker(this)); diff --git a/packages/grpc-js/src/picker.ts b/packages/grpc-js/src/picker.ts index d95eca21b..6474269f7 100644 --- a/packages/grpc-js/src/picker.ts +++ b/packages/grpc-js/src/picker.ts @@ -97,16 +97,13 @@ export interface Picker { */ export class UnavailablePicker implements Picker { private status: StatusObject; - constructor(status?: StatusObject) { - if (status !== undefined) { - this.status = status; - } else { - this.status = { - code: Status.UNAVAILABLE, - details: 'No connection established', - metadata: new Metadata(), - }; - } + constructor(status?: Partial) { + this.status = { + code: Status.UNAVAILABLE, + details: 'No connection established', + metadata: new Metadata(), + ...status, + }; } pick(pickArgs: PickArgs): TransientFailurePickResult { return { diff --git a/packages/grpc-js/src/subchannel-interface.ts b/packages/grpc-js/src/subchannel-interface.ts index 9b947ad32..cc19c22c4 100644 --- a/packages/grpc-js/src/subchannel-interface.ts +++ b/packages/grpc-js/src/subchannel-interface.ts @@ -23,7 +23,8 @@ export type ConnectivityStateListener = ( subchannel: SubchannelInterface, previousState: ConnectivityState, newState: ConnectivityState, - keepaliveTime: number + keepaliveTime: number, + errorMessage?: string ) => void; /** diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index 0f8844720..03bbf035f 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -250,7 +250,8 @@ export class Subchannel { error => { this.transitionToState( [ConnectivityState.CONNECTING], - ConnectivityState.TRANSIENT_FAILURE + ConnectivityState.TRANSIENT_FAILURE, + `${error}` ); } ); @@ -265,7 +266,8 @@ export class Subchannel { */ private transitionToState( oldStates: ConnectivityState[], - newState: ConnectivityState + newState: ConnectivityState, + errorMessage?: string ): boolean { if (oldStates.indexOf(this.connectivityState) === -1) { return false; @@ -318,7 +320,7 @@ export class Subchannel { throw new Error(`Invalid state: unknown ConnectivityState ${newState}`); } for (const listener of this.stateListeners) { - listener(this, previousState, newState, this.keepaliveTime); + listener(this, previousState, newState, this.keepaliveTime, errorMessage); } return true; } diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 49ec01ddc..8dc4e2de6 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -741,6 +741,7 @@ export class Http2SubchannelConnector implements SubchannelConnector { connectionOptions ); this.session = session; + let errorMessage = 'Failed to connect'; session.unref(); session.once('connect', () => { session.removeAllListeners(); @@ -749,10 +750,14 @@ export class Http2SubchannelConnector implements SubchannelConnector { }); session.once('close', () => { this.session = null; - reject(); + // Leave time for error event to happen before rejecting + setImmediate(() => { + reject(`${errorMessage} (${new Date().toISOString()})`); + }); }); session.once('error', error => { - this.trace('connection failed with error ' + (error as Error).message); + errorMessage = (error as Error).message; + this.trace('connection failed with error ' + errorMessage); }); }); } From 2f5ddc713774e863eef5c708e4f666b3a60d81ef Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 19 Oct 2023 13:57:57 -0700 Subject: [PATCH 25/38] grpc-js: pick_first: fix happy eyeballs and reresolution in sticky TF mode --- packages/grpc-js/package.json | 2 +- .../grpc-js/src/load-balancer-pick-first.ts | 30 +++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 9d66b4095..29e4b5849 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.6", + "version": "1.9.7", "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/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 1a22b3f76..5e8361ace 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -174,6 +174,12 @@ export class PickFirstLoadBalancer implements LoadBalancer { */ private stickyTransientFailureMode = false; + /** + * Indicates whether we called channelControlHelper.requestReresolution since + * the last call to updateAddressList + */ + private requestedResolutionSinceLastUpdate = false; + /** * The most recent error reported by any subchannel as it transitioned to * TRANSIENT_FAILURE. @@ -216,15 +222,28 @@ export class PickFirstLoadBalancer implements LoadBalancer { } } + private requestReresolution() { + this.requestedResolutionSinceLastUpdate = true; + this.channelControlHelper.requestReresolution(); + } + private maybeEnterStickyTransientFailureMode() { - if (this.stickyTransientFailureMode) { + if (!this.allChildrenHaveReportedTF()) { return; } - if (!this.allChildrenHaveReportedTF()) { + if (!this.requestedResolutionSinceLastUpdate) { + /* Each time we get an update we reset each subchannel's + * hasReportedTransientFailure flag, so the next time we get to this + * point after that, each subchannel has reported TRANSIENT_FAILURE + * at least once since then. That is the trigger for requesting + * reresolution, whether or not the LB policy is already in sticky TF + * mode. */ + this.requestReresolution(); + } + if (this.stickyTransientFailureMode) { return; } this.stickyTransientFailureMode = true; - this.channelControlHelper.requestReresolution(); for (const { subchannel } of this.children) { subchannel.startConnecting(); } @@ -256,7 +275,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { if (newState !== ConnectivityState.READY) { this.removeCurrentPick(); this.calculateAndReportNewState(); - this.channelControlHelper.requestReresolution(); + this.requestReresolution(); } return; } @@ -283,7 +302,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { private startNextSubchannelConnecting(startIndex: number) { clearTimeout(this.connectionDelayTimeout); - if (this.triedAllSubchannels || this.stickyTransientFailureMode) { + if (this.triedAllSubchannels) { return; } for (const [index, child] of this.children.entries()) { @@ -382,6 +401,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { this.currentSubchannelIndex = 0; this.children = []; this.triedAllSubchannels = false; + this.requestedResolutionSinceLastUpdate = false; } updateAddressList( From d465f839d46d708ca93a06956181a7e27b4e7ba0 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 19 Oct 2023 16:20:04 -0700 Subject: [PATCH 26/38] Add pick_first requestReresolution tests --- packages/grpc-js/test/test-pick-first.ts | 90 +++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/test/test-pick-first.ts b/packages/grpc-js/test/test-pick-first.ts index e9e9e5601..a018e7a2d 100644 --- a/packages/grpc-js/test/test-pick-first.ts +++ b/packages/grpc-js/test/test-pick-first.ts @@ -41,7 +41,11 @@ function updateStateCallBackForExpectedStateSequence( ) { const actualStateSequence: ConnectivityState[] = []; let lastPicker: Picker | null = null; + let finished = false; return (connectivityState: ConnectivityState, picker: Picker) => { + if (finished) { + return; + } // Ignore duplicate state transitions if ( connectivityState === actualStateSequence[actualStateSequence.length - 1] @@ -60,6 +64,7 @@ function updateStateCallBackForExpectedStateSequence( if ( expectedStateSequence[actualStateSequence.length] !== connectivityState ) { + finished = true; done( new Error( `Unexpected state ${ @@ -69,10 +74,12 @@ function updateStateCallBackForExpectedStateSequence( )}]` ) ); + return; } actualStateSequence.push(connectivityState); lastPicker = picker; if (actualStateSequence.length === expectedStateSequence.length) { + finished = true; done(); } }; @@ -90,7 +97,7 @@ describe('Shuffler', () => { }); }); -describe('pick_first load balancing policy', () => { +describe.only('pick_first load balancing policy', () => { const config = new PickFirstLoadBalancingConfig(false); let subchannels: MockSubchannel[] = []; const baseChannelControlHelper: ChannelControlHelper = { @@ -462,6 +469,87 @@ describe('pick_first load balancing policy', () => { }); }); }); + it('Should request reresolution every time each child reports TF', done => { + let reresolutionRequestCount = 0; + const targetReresolutionRequestCount = 3; + const currentStartState = ConnectivityState.IDLE; + const channelControlHelper = createChildChannelControlHelper( + baseChannelControlHelper, + { + createSubchannel: (subchannelAddress, subchannelArgs) => { + const subchannel = new MockSubchannel( + subchannelAddressToString(subchannelAddress), + currentStartState + ); + subchannels.push(subchannel); + return subchannel; + }, + updateState: updateStateCallBackForExpectedStateSequence( + [ConnectivityState.CONNECTING, ConnectivityState.TRANSIENT_FAILURE], + err => setImmediate(() => { + assert.strictEqual(reresolutionRequestCount, targetReresolutionRequestCount); + done(err); + }) + ), + requestReresolution: () => { + reresolutionRequestCount += 1; + } + } + ); + const pickFirst = new PickFirstLoadBalancer(channelControlHelper); + pickFirst.updateAddressList([{ host: 'localhost', port: 1 }], config); + process.nextTick(() => { + subchannels[0].transitionToState(ConnectivityState.TRANSIENT_FAILURE); + process.nextTick(() => { + pickFirst.updateAddressList([{ host: 'localhost', port: 2 }], config); + process.nextTick(() => { + subchannels[1].transitionToState(ConnectivityState.TRANSIENT_FAILURE); + process.nextTick(() => { + pickFirst.updateAddressList([{ host: 'localhost', port: 3 }], config); + process.nextTick(() => { + subchannels[2].transitionToState(ConnectivityState.TRANSIENT_FAILURE); + }); + }); + }); + }); + }); + }); + it('Should request reresolution if the new subchannels are already in TF', done => { + let reresolutionRequestCount = 0; + const targetReresolutionRequestCount = 3; + const currentStartState = ConnectivityState.TRANSIENT_FAILURE; + const channelControlHelper = createChildChannelControlHelper( + baseChannelControlHelper, + { + createSubchannel: (subchannelAddress, subchannelArgs) => { + const subchannel = new MockSubchannel( + subchannelAddressToString(subchannelAddress), + currentStartState + ); + subchannels.push(subchannel); + return subchannel; + }, + updateState: updateStateCallBackForExpectedStateSequence( + [ConnectivityState.TRANSIENT_FAILURE], + err => setImmediate(() => { + assert.strictEqual(reresolutionRequestCount, targetReresolutionRequestCount); + done(err); + }) + ), + requestReresolution: () => { + reresolutionRequestCount += 1; + } + } + ); + const pickFirst = new PickFirstLoadBalancer(channelControlHelper); + pickFirst.updateAddressList([{ host: 'localhost', port: 1 }], config); + process.nextTick(() => { + pickFirst.updateAddressList([{ host: 'localhost', port: 2 }], config); + process.nextTick(() => { + pickFirst.updateAddressList([{ host: 'localhost', port: 2 }], config); + }); + }); + }); describe('Address list randomization', () => { const shuffleConfig = new PickFirstLoadBalancingConfig(true); it('Should pick different subchannels after multiple updates', done => { From 446f139b37c6d446dd4d322043747018b760d80e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 27 Oct 2023 10:14:58 -0700 Subject: [PATCH 27/38] grpc-js: Cancel and don't start idle timer on shutdown --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/internal-channel.ts | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 29e4b5849..6627b0103 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.7", + "version": "1.9.8", "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/internal-channel.ts b/packages/grpc-js/src/internal-channel.ts index 0ed189c03..f36849ef8 100644 --- a/packages/grpc-js/src/internal-channel.ts +++ b/packages/grpc-js/src/internal-channel.ts @@ -554,7 +554,7 @@ export class InternalChannel { } private maybeStartIdleTimer() { - if (this.callCount === 0) { + if (this.connectivityState !== ConnectivityState.SHUTDOWN && this.callCount === 0) { this.idleTimer = setTimeout(() => { this.trace( 'Idle timer triggered after ' + @@ -706,6 +706,9 @@ export class InternalChannel { this.resolvingLoadBalancer.destroy(); this.updateState(ConnectivityState.SHUTDOWN); clearInterval(this.callRefTimer); + if (this.idleTimer) { + clearTimeout(this.idleTimer); + } if (this.channelzEnabled) { unregisterChannelzRef(this.channelzRef); } From 9050ea9dae8b5f015f2d9ecd12fb4432ab953eef Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 30 Oct 2023 09:42:29 -0700 Subject: [PATCH 28/38] grpc-js: Don't repeat fixed resolver results --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/resolver-dns.ts | 25 ++++++++++-------- packages/grpc-js/src/resolver-ip.ts | 32 +++++++++++++----------- packages/grpc-js/test/test-pick-first.ts | 2 +- 4 files changed, 35 insertions(+), 26 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 6627b0103..56fa0d3ff 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.8", + "version": "1.9.9", "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 149530bbd..9431183d3 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -99,6 +99,7 @@ class DnsResolver implements Resolver { private nextResolutionTimer: NodeJS.Timeout; private isNextResolutionTimerRunning = false; private isServiceConfigEnabled = true; + private returnedIpResult = false; constructor( private target: GrpcUri, private listener: ResolverListener, @@ -163,16 +164,19 @@ class DnsResolver implements Resolver { */ private startResolution() { if (this.ipResult !== null) { - trace('Returning IP address for target ' + uriToString(this.target)); - setImmediate(() => { - this.listener.onSuccessfulResolution( - this.ipResult!, - null, - null, - null, - {} - ); - }); + if (!this.returnedIpResult) { + trace('Returning IP address for target ' + uriToString(this.target)); + setImmediate(() => { + this.listener.onSuccessfulResolution( + this.ipResult!, + null, + null, + null, + {} + ); + }); + this.returnedIpResult = true; + } this.backoff.stop(); this.backoff.reset(); this.stopNextResolutionTimer(); @@ -380,6 +384,7 @@ class DnsResolver implements Resolver { this.latestLookupResult = null; this.latestServiceConfig = null; this.latestServiceConfigError = null; + this.returnedIpResult = false; } /** diff --git a/packages/grpc-js/src/resolver-ip.ts b/packages/grpc-js/src/resolver-ip.ts index 0704131e1..7cf4f5645 100644 --- a/packages/grpc-js/src/resolver-ip.ts +++ b/packages/grpc-js/src/resolver-ip.ts @@ -41,6 +41,7 @@ const DEFAULT_PORT = 443; class IpResolver implements Resolver { private addresses: SubchannelAddress[] = []; private error: StatusObject | null = null; + private hasReturnedResult = false; constructor( target: GrpcUri, private listener: ResolverListener, @@ -87,22 +88,25 @@ class IpResolver implements Resolver { trace('Parsed ' + target.scheme + ' address list ' + this.addresses); } updateResolution(): void { - process.nextTick(() => { - if (this.error) { - this.listener.onError(this.error); - } else { - this.listener.onSuccessfulResolution( - this.addresses, - null, - null, - null, - {} - ); - } - }); + if (!this.hasReturnedResult) { + this.hasReturnedResult = true; + process.nextTick(() => { + if (this.error) { + this.listener.onError(this.error); + } else { + this.listener.onSuccessfulResolution( + this.addresses, + null, + null, + null, + {} + ); + } + }); + } } destroy(): void { - // This resolver owns no resources, so we do nothing here. + this.hasReturnedResult = false; } static getDefaultAuthority(target: GrpcUri): string { diff --git a/packages/grpc-js/test/test-pick-first.ts b/packages/grpc-js/test/test-pick-first.ts index a018e7a2d..075448882 100644 --- a/packages/grpc-js/test/test-pick-first.ts +++ b/packages/grpc-js/test/test-pick-first.ts @@ -97,7 +97,7 @@ describe('Shuffler', () => { }); }); -describe.only('pick_first load balancing policy', () => { +describe('pick_first load balancing policy', () => { const config = new PickFirstLoadBalancingConfig(false); let subchannels: MockSubchannel[] = []; const baseChannelControlHelper: ChannelControlHelper = { From 1f148e93496f4fb7ed245ed7c5c3b157552b16b4 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 27 Oct 2023 19:28:37 +0300 Subject: [PATCH 29/38] Fix missing port in proxy CONNECT when using the default HTTPS port --- packages/grpc-js/src/http_proxy.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/src/http_proxy.ts b/packages/grpc-js/src/http_proxy.ts index 3aed28c85..d1df22d2b 100644 --- a/packages/grpc-js/src/http_proxy.ts +++ b/packages/grpc-js/src/http_proxy.ts @@ -189,12 +189,16 @@ export function getProxiedConnection( if (parsedTarget === null) { return Promise.resolve({}); } + const targetHostPost = splitHostPort(parsedTarget.path); + if (targetHostPost === null) { + return Promise.resolve({}); + } const options: http.RequestOptions = { method: 'CONNECT', - path: parsedTarget.path, + path: targetHostPost.host + ':' + (targetHostPost.port != null ? targetHostPost.port : '443'), }; const headers: http.OutgoingHttpHeaders = { - Host: parsedTarget.path, + Host: targetHostPost.host + ':' + (targetHostPost.port != null ? targetHostPost.port : '443'), }; // Connect to the subchannel address as a proxy if (isTcpSubchannelAddress(address)) { From 0854192dbaf3e305fb450f29ee0bfda57356668a Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Tue, 31 Oct 2023 00:19:32 +0200 Subject: [PATCH 30/38] Review fixes --- packages/grpc-js/src/http_proxy.ts | 12 ++++++++---- packages/grpc-js/src/resolver-dns.ts | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js/src/http_proxy.ts b/packages/grpc-js/src/http_proxy.ts index d1df22d2b..3e905c488 100644 --- a/packages/grpc-js/src/http_proxy.ts +++ b/packages/grpc-js/src/http_proxy.ts @@ -30,6 +30,7 @@ import { import { ChannelOptions } from './channel-options'; import { GrpcUri, parseUri, splitHostPort, uriToString } from './uri-parser'; import { URL } from 'url'; +import { DEFAULT_PORT } from './resolver-dns'; const TRACER_NAME = 'proxy'; @@ -189,16 +190,19 @@ export function getProxiedConnection( if (parsedTarget === null) { return Promise.resolve({}); } - const targetHostPost = splitHostPort(parsedTarget.path); - if (targetHostPost === null) { + const splitHostPost = splitHostPort(parsedTarget.path); + if (splitHostPost === null) { return Promise.resolve({}); } + const hostPort = `${splitHostPost.host}:${ + splitHostPost.port ?? DEFAULT_PORT + }`; const options: http.RequestOptions = { method: 'CONNECT', - path: targetHostPost.host + ':' + (targetHostPost.port != null ? targetHostPost.port : '443'), + path: hostPort, }; const headers: http.OutgoingHttpHeaders = { - Host: targetHostPost.host + ':' + (targetHostPost.port != null ? targetHostPost.port : '443'), + Host: hostPort, }; // Connect to the subchannel address as a proxy if (isTcpSubchannelAddress(address)) { diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 9431183d3..31e0d0bab 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -43,7 +43,7 @@ function trace(text: string): void { /** * The default TCP port to connect to if not explicitly specified in the target. */ -const DEFAULT_PORT = 443; +export const DEFAULT_PORT = 443; const DEFAULT_MIN_TIME_BETWEEN_RESOLUTIONS_MS = 30_000; From bf2009a72f7b071cb9b1605ca24646b01e9c2621 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 7 Nov 2023 11:09:59 -0800 Subject: [PATCH 31/38] grpc-js: Handle unset opaqueData in goaway event --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/transport.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 56fa0d3ff..be29ff870 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.9", + "version": "1.9.10", "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/transport.ts b/packages/grpc-js/src/transport.ts index 8dc4e2de6..39ca69383 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -194,17 +194,18 @@ class Http2Transport implements Transport { }); session.once( 'goaway', - (errorCode: number, lastStreamID: number, opaqueData: Buffer) => { + (errorCode: number, lastStreamID: number, opaqueData?: Buffer) => { let tooManyPings = false; /* See the last paragraph of * https://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md#basic-keepalive */ if ( errorCode === http2.constants.NGHTTP2_ENHANCE_YOUR_CALM && + opaqueData && opaqueData.equals(tooManyPingsData) ) { tooManyPings = true; } - this.trace('connection closed by GOAWAY with code ' + errorCode); + this.trace('connection closed by GOAWAY with code ' + errorCode + ' and data ' + opaqueData?.toString()); this.reportDisconnectToOwner(tooManyPings); } ); From 8843706ec7a42efb2a083e565f661b667dbbb589 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 16 Nov 2023 10:15:48 -0800 Subject: [PATCH 32/38] grpc-js: Make pick_first use exitIdle --- packages/grpc-js/package.json | 2 +- .../grpc-js/src/load-balancer-pick-first.ts | 39 +++++++++++-------- packages/grpc-js/test/test-pick-first.ts | 28 +++++++++++++ 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index be29ff870..4c408873e 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.10", + "version": "1.9.11", "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/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 5e8361ace..214f92e22 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -186,6 +186,8 @@ export class PickFirstLoadBalancer implements LoadBalancer { */ private lastError: string | null = null; + private latestAddressList: SubchannelAddress[] | null = null; + /** * Load balancer that attempts to connect to each backend in the address list * in order, and picks the first one that connects, using it for every @@ -404,19 +406,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { this.requestedResolutionSinceLastUpdate = false; } - updateAddressList( - addressList: SubchannelAddress[], - lbConfig: LoadBalancingConfig - ): void { - if (!(lbConfig instanceof PickFirstLoadBalancingConfig)) { - return; - } - /* Previously, an update would be discarded if it was identical to the - * previous update, to minimize churn. Now the DNS resolver is - * rate-limited, so that is less of a concern. */ - if (lbConfig.getShuffleAddressList()) { - addressList = shuffled(addressList); - } + private connectToAddressList(addressList: SubchannelAddress[]) { const newChildrenList = addressList.map(address => ({ subchannel: this.channelControlHelper.createSubchannel(address, {}), hasReportedTransientFailure: false, @@ -449,10 +439,27 @@ export class PickFirstLoadBalancer implements LoadBalancer { this.calculateAndReportNewState(); } + updateAddressList( + addressList: SubchannelAddress[], + lbConfig: LoadBalancingConfig + ): void { + if (!(lbConfig instanceof PickFirstLoadBalancingConfig)) { + return; + } + /* Previously, an update would be discarded if it was identical to the + * previous update, to minimize churn. Now the DNS resolver is + * rate-limited, so that is less of a concern. */ + if (lbConfig.getShuffleAddressList()) { + addressList = shuffled(addressList); + } + this.latestAddressList = addressList; + this.connectToAddressList(addressList); + } + exitIdle() { - /* The pick_first LB policy is only in the IDLE state if it has no - * addresses to try to connect to and it has no picked subchannel. - * In that case, there is no meaningful action that can be taken here. */ + if (this.currentState === ConnectivityState.IDLE && this.latestAddressList) { + this.connectToAddressList(this.latestAddressList); + } } resetBackoff() { diff --git a/packages/grpc-js/test/test-pick-first.ts b/packages/grpc-js/test/test-pick-first.ts index 075448882..30455fcb8 100644 --- a/packages/grpc-js/test/test-pick-first.ts +++ b/packages/grpc-js/test/test-pick-first.ts @@ -550,6 +550,34 @@ describe('pick_first load balancing policy', () => { }); }); }); + it('Should reconnect to the same address list if exitIdle is called', done => { + const currentStartState = ConnectivityState.READY; + const channelControlHelper = createChildChannelControlHelper( + baseChannelControlHelper, + { + createSubchannel: (subchannelAddress, subchannelArgs) => { + const subchannel = new MockSubchannel( + subchannelAddressToString(subchannelAddress), + currentStartState + ); + subchannels.push(subchannel); + return subchannel; + }, + updateState: updateStateCallBackForExpectedStateSequence( + [ConnectivityState.READY, ConnectivityState.IDLE, ConnectivityState.READY], + done + ), + } + ); + const pickFirst = new PickFirstLoadBalancer(channelControlHelper); + pickFirst.updateAddressList([{ host: 'localhost', port: 1 }], config); + process.nextTick(() => { + subchannels[0].transitionToState(ConnectivityState.IDLE); + process.nextTick(() => { + pickFirst.exitIdle(); + }); + }); + }); describe('Address list randomization', () => { const shuffleConfig = new PickFirstLoadBalancingConfig(true); it('Should pick different subchannels after multiple updates', done => { From 736d6df80b0065d5048b7bcadeb44317898a08cb Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 16 Nov 2023 10:19:26 -0800 Subject: [PATCH 33/38] grpc-js: Return the result from the UDS resolver only once --- packages/grpc-js/src/resolver-uds.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/grpc-js/src/resolver-uds.ts b/packages/grpc-js/src/resolver-uds.ts index 24095ec29..2110e8564 100644 --- a/packages/grpc-js/src/resolver-uds.ts +++ b/packages/grpc-js/src/resolver-uds.ts @@ -21,6 +21,7 @@ import { ChannelOptions } from './channel-options'; class UdsResolver implements Resolver { private addresses: SubchannelAddress[] = []; + private hasReturnedResult = false; constructor( target: GrpcUri, private listener: ResolverListener, @@ -35,14 +36,17 @@ class UdsResolver implements Resolver { this.addresses = [{ path }]; } updateResolution(): void { - process.nextTick( - this.listener.onSuccessfulResolution, - this.addresses, - null, - null, - null, - {} - ); + if (!this.hasReturnedResult) { + this.hasReturnedResult = true; + process.nextTick( + this.listener.onSuccessfulResolution, + this.addresses, + null, + null, + null, + {} + ); + } } destroy() { From 6d4e08cfd40cf755d55b4871777c16e4e46ecaee Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 27 Nov 2023 12:03:12 -0500 Subject: [PATCH 34/38] grpc-js: pick_first: fix currentPick comparison in resetSubchannelList --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/load-balancer-pick-first.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 4c408873e..465f56fb3 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.11", + "version": "1.9.12", "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/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 214f92e22..75c9d4cfe 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -382,7 +382,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { private resetSubchannelList() { for (const child of this.children) { - if (child.subchannel !== this.currentPick) { + if (!(this.currentPick && child.subchannel.realSubchannelEquals(this.currentPick))) { /* The connectivity state listener is the same whether the subchannel * is in the list of children or it is the currentPick, so if it is in * both, removing it here would cause problems. In particular, that From 4dfd8c43d73231aa2d1b9e26cc0bdb2ff6525328 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 1 Dec 2023 10:26:49 -0500 Subject: [PATCH 35/38] grpc-js: Fix call ref timer handling --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/internal-channel.ts | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 465f56fb3..78c404c94 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.12", + "version": "1.9.13", "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/internal-channel.ts b/packages/grpc-js/src/internal-channel.ts index f36849ef8..ff3ccb5f9 100644 --- a/packages/grpc-js/src/internal-channel.ts +++ b/packages/grpc-js/src/internal-channel.ts @@ -296,7 +296,9 @@ export class InternalChannel { this.currentPicker = picker; const queueCopy = this.pickQueue.slice(); this.pickQueue = []; - this.callRefTimerUnref(); + if (queueCopy.length > 0) { + this.callRefTimerUnref(); + } for (const call of queueCopy) { call.doPick(); } @@ -349,11 +351,12 @@ export class InternalChannel { process.nextTick(() => { const localQueue = this.configSelectionQueue; this.configSelectionQueue = []; - this.callRefTimerUnref(); + if (localQueue.length > 0) { + this.callRefTimerUnref(); + } for (const call of localQueue) { call.getConfig(); } - this.configSelectionQueue = []; }); }, status => { @@ -380,7 +383,9 @@ export class InternalChannel { } const localQueue = this.configSelectionQueue; this.configSelectionQueue = []; - this.callRefTimerUnref(); + if (localQueue.length > 0) { + this.callRefTimerUnref(); + } for (const call of localQueue) { call.reportResolverError(status); } From 6e6f942f1918738ea2c4b0abc7d2a30befcdfbbc Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 4 Jan 2024 13:11:54 -0800 Subject: [PATCH 36/38] Merge pull request #2635 from XuanWang-Amos/psm-interop-shared-build buildscripts: Use the Kokoro shared install lib from the new repo --- packages/grpc-js-xds/scripts/xds_k8s_lb.sh | 2 +- packages/grpc-js-xds/scripts/xds_k8s_url_map.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js-xds/scripts/xds_k8s_lb.sh b/packages/grpc-js-xds/scripts/xds_k8s_lb.sh index 729fb9293..ed7c77fe2 100755 --- a/packages/grpc-js-xds/scripts/xds_k8s_lb.sh +++ b/packages/grpc-js-xds/scripts/xds_k8s_lb.sh @@ -17,7 +17,7 @@ set -eo pipefail # Constants readonly GITHUB_REPOSITORY_NAME="grpc-node" -readonly TEST_DRIVER_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/${TEST_DRIVER_REPO_OWNER:-grpc}/grpc/${TEST_DRIVER_BRANCH:-master}/tools/internal_ci/linux/grpc_xds_k8s_install_test_driver.sh" +readonly TEST_DRIVER_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/${TEST_DRIVER_REPO_OWNER:-grpc}/psm-interop/${TEST_DRIVER_BRANCH:-main}/.kokoro/psm_interop_kokoro_lib.sh" ## xDS test client Docker images readonly SERVER_IMAGE_NAME="gcr.io/grpc-testing/xds-interop/java-server:558b5b0bfac8e21755c223063274a779b3898afe" readonly CLIENT_IMAGE_NAME="gcr.io/grpc-testing/xds-interop/node-client" diff --git a/packages/grpc-js-xds/scripts/xds_k8s_url_map.sh b/packages/grpc-js-xds/scripts/xds_k8s_url_map.sh index fc74718f2..9344d054b 100644 --- a/packages/grpc-js-xds/scripts/xds_k8s_url_map.sh +++ b/packages/grpc-js-xds/scripts/xds_k8s_url_map.sh @@ -17,7 +17,7 @@ set -eo pipefail # Constants readonly GITHUB_REPOSITORY_NAME="grpc-node" -readonly TEST_DRIVER_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/${TEST_DRIVER_REPO_OWNER:-grpc}/grpc/${TEST_DRIVER_BRANCH:-master}/tools/internal_ci/linux/grpc_xds_k8s_install_test_driver.sh" +readonly TEST_DRIVER_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/${TEST_DRIVER_REPO_OWNER:-grpc}/psm-interop/${TEST_DRIVER_BRANCH:-main}/.kokoro/psm_interop_kokoro_lib.sh" ## xDS test client Docker images readonly CLIENT_IMAGE_NAME="gcr.io/grpc-testing/xds-interop/node-client" readonly FORCE_IMAGE_BUILD="${FORCE_IMAGE_BUILD:-0}" From 6da0b49dbc7efd201d464397ec626e4db01a6560 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 16 Jan 2024 14:18:05 -0800 Subject: [PATCH 37/38] grpc-js: Fix and optimize IDLE timeouts --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/internal-channel.ts | 43 +++++++++++++++++++----- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 78c404c94..8d8f4fd90 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.9.13", + "version": "1.9.14", "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/internal-channel.ts b/packages/grpc-js/src/internal-channel.ts index ff3ccb5f9..6a65b712f 100644 --- a/packages/grpc-js/src/internal-channel.ts +++ b/packages/grpc-js/src/internal-channel.ts @@ -184,6 +184,7 @@ export class InternalChannel { private callCount = 0; private idleTimer: NodeJS.Timeout | null = null; private readonly idleTimeoutMs: number; + private lastActivityTimestamp: Date; // Channelz info private readonly channelzEnabled: boolean = true; @@ -409,6 +410,7 @@ export class InternalChannel { 'Channel constructed \n' + error.stack?.substring(error.stack.indexOf('\n') + 1) ); + this.lastActivityTimestamp = new Date(); } private getChannelzInfo(): ChannelInfo { @@ -556,19 +558,44 @@ export class InternalChannel { this.resolvingLoadBalancer.destroy(); this.updateState(ConnectivityState.IDLE); this.currentPicker = new QueuePicker(this.resolvingLoadBalancer); + if (this.idleTimer) { + clearTimeout(this.idleTimer); + this.idleTimer = null; + } } - private maybeStartIdleTimer() { - if (this.connectivityState !== ConnectivityState.SHUTDOWN && this.callCount === 0) { - this.idleTimer = setTimeout(() => { + private startIdleTimeout(timeoutMs: number) { + this.idleTimer = setTimeout(() => { + if (this.callCount > 0) { + /* If there is currently a call, the channel will not go idle for a + * period of at least idleTimeoutMs, so check again after that time. + */ + this.startIdleTimeout(this.idleTimeoutMs); + return; + } + const now = new Date(); + const timeSinceLastActivity = now.valueOf() - this.lastActivityTimestamp.valueOf(); + if (timeSinceLastActivity >= this.idleTimeoutMs) { this.trace( 'Idle timer triggered after ' + this.idleTimeoutMs + 'ms of inactivity' ); this.enterIdle(); - }, this.idleTimeoutMs); - this.idleTimer.unref?.(); + } else { + /* Whenever the timer fires with the latest activity being too recent, + * set the timer again for the time when the time since the last + * activity is equal to the timeout. This should result in the timer + * firing no more than once every idleTimeoutMs/2 on average. */ + this.startIdleTimeout(this.idleTimeoutMs - timeSinceLastActivity); + } + }, timeoutMs); + this.idleTimer.unref?.(); + } + + private maybeStartIdleTimer() { + if (this.connectivityState !== ConnectivityState.SHUTDOWN && !this.idleTimer) { + this.startIdleTimeout(this.idleTimeoutMs); } } @@ -577,10 +604,6 @@ export class InternalChannel { this.callTracker.addCallStarted(); } this.callCount += 1; - if (this.idleTimer) { - clearTimeout(this.idleTimer); - this.idleTimer = null; - } } private onCallEnd(status: StatusObject) { @@ -592,6 +615,7 @@ export class InternalChannel { } } this.callCount -= 1; + this.lastActivityTimestamp = new Date(); this.maybeStartIdleTimer(); } @@ -729,6 +753,7 @@ export class InternalChannel { const connectivityState = this.connectivityState; if (tryToConnect) { this.resolvingLoadBalancer.exitIdle(); + this.lastActivityTimestamp = new Date(); this.maybeStartIdleTimer(); } return connectivityState; From 2b31f8c148b790ca8df4deb1fa644cbbc465f264 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 16 Jan 2024 14:37:55 -0800 Subject: [PATCH 38/38] grpc-js: Shutdown transport if a state change occurs while connecting --- packages/grpc-js/src/subchannel.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/grpc-js/src/subchannel.ts b/packages/grpc-js/src/subchannel.ts index 03bbf035f..eff7509a5 100644 --- a/packages/grpc-js/src/subchannel.ts +++ b/packages/grpc-js/src/subchannel.ts @@ -245,6 +245,10 @@ export class Subchannel { ); } }); + } else { + /* If we can't transition from CONNECTING to READY here, we will + * not be using this transport, so release its resources. */ + transport.shutdown(); } }, error => {