From 2ee8911dc4bca2937c08ed3864001ffa81a9adf3 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 12 Jul 2024 13:43:33 -0700 Subject: [PATCH 01/12] grpc-js: Bump packages to 1.11.0, and update documentation --- packages/grpc-js-xds/README.md | 1 + packages/grpc-js-xds/package.json | 4 ++-- packages/grpc-js/README.md | 1 + packages/grpc-js/package.json | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/grpc-js-xds/README.md b/packages/grpc-js-xds/README.md index 5b7a5400a..48a4db10f 100644 --- a/packages/grpc-js-xds/README.md +++ b/packages/grpc-js-xds/README.md @@ -35,3 +35,4 @@ const client = new MyServiceClient('xds:///example.com:123'); - [xDS Custom Load Balancer Configuration](https://github.com/grpc/proposal/blob/master/A52-xds-custom-lb-policies.md) (Custom load balancer registration not currently supported) - [xDS Ring Hash LB Policy](https://github.com/grpc/proposal/blob/master/A42-xds-ring-hash-lb-policy.md) - [`pick_first` via xDS](https://github.com/grpc/proposal/blob/master/A62-pick-first.md#pick_first-via-xds-1) (Currently experimental, enabled by environment variable `GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG`) + - [xDS-Enabled Servers](https://github.com/grpc/proposal/blob/master/A36-xds-for-servers.md) diff --git a/packages/grpc-js-xds/package.json b/packages/grpc-js-xds/package.json index c2b421cec..3bd247b0b 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.10.1", + "version": "1.11.0", "description": "Plugin for @grpc/grpc-js. Adds the xds:// URL scheme and associated features.", "main": "build/src/index.js", "scripts": { @@ -52,7 +52,7 @@ "xxhash-wasm": "^1.0.2" }, "peerDependencies": { - "@grpc/grpc-js": "~1.10.0" + "@grpc/grpc-js": "~1.11.0" }, "engines": { "node": ">=10.10.0" diff --git a/packages/grpc-js/README.md b/packages/grpc-js/README.md index f3b682f3c..dfe805021 100644 --- a/packages/grpc-js/README.md +++ b/packages/grpc-js/README.md @@ -69,6 +69,7 @@ Many channel arguments supported in `grpc` are not supported in `@grpc/grpc-js`. - `grpc.client_idle_timeout_ms` - `grpc-node.max_session_memory` - `grpc-node.tls_enable_trace` + - `grpc-node.retry_max_attempts_limit` - `channelOverride` - `channelFactoryOverride` diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index f0c5da780..353cbe258 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.10.11", + "version": "1.11.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 996a6375c6499146e8fe2c962803b915adec5952 Mon Sep 17 00:00:00 2001 From: xqin Date: Tue, 16 Jul 2024 16:06:59 +0800 Subject: [PATCH 02/12] support node v14 again --- packages/grpc-js/src/resolver-dns.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 56f3dc1e8..9e7b8bbfb 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -20,7 +20,7 @@ import { registerResolver, registerDefaultScheme, } from './resolver'; -import { promises as dns } from 'node:dns'; +import { promises as dns } from 'dns'; import { extractAndSelectServiceConfig, ServiceConfig } from './service-config'; import { Status } from './constants'; import { StatusObject } from './call-interface'; From 2ecd53d8155f9564e352a669dc5fe3613f1afe14 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 16 Jul 2024 10:15:16 -0700 Subject: [PATCH 03/12] grpc-js: Bump to 1.11.1 --- 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 353cbe258..19c3aea87 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.11.0", + "version": "1.11.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", From 15d422d5bb35d4108973494433cead7539cabd0a Mon Sep 17 00:00:00 2001 From: hastom Date: Thu, 18 Jul 2024 12:57:40 +0300 Subject: [PATCH 04/12] Fix client crash on custom error code --- packages/grpc-js/src/retrying-call.ts | 2 +- packages/grpc-js/test/test-retry.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/src/retrying-call.ts b/packages/grpc-js/src/retrying-call.ts index dbc036e42..fcc6865de 100644 --- a/packages/grpc-js/src/retrying-call.ts +++ b/packages/grpc-js/src/retrying-call.ts @@ -398,7 +398,7 @@ export class RetryingCall implements Call, DeadlineInfoProvider { return list.some( value => value === code || - value.toString().toLowerCase() === Status[code].toLowerCase() + value.toString().toLowerCase() === Status[code]?.toLowerCase() ); } diff --git a/packages/grpc-js/test/test-retry.ts b/packages/grpc-js/test/test-retry.ts index 0f76aae19..627836819 100644 --- a/packages/grpc-js/test/test-retry.ts +++ b/packages/grpc-js/test/test-retry.ts @@ -323,6 +323,21 @@ describe('Retries', () => { } ); }); + + it('Should not retry on custom error code', done => { + const metadata = new grpc.Metadata(); + metadata.set('succeed-on-retry-attempt', '2'); + metadata.set('respond-with-status', '300'); + client.echo( + { value: 'test value', value2: 3 }, + metadata, + (error: grpc.ServiceError, response: any) => { + assert(error); + assert.strictEqual(error.details, 'Failed on retry 0'); + done(); + } + ); + }); }); describe('Client with hedging configured', () => { From 33073d0db2d05e79d26022f00def47f26335b131 Mon Sep 17 00:00:00 2001 From: hastom Date: Fri, 19 Jul 2024 13:24:07 +0300 Subject: [PATCH 05/12] Fix test case --- packages/grpc-js/test/test-retry.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/grpc-js/test/test-retry.ts b/packages/grpc-js/test/test-retry.ts index 627836819..26ad26c2a 100644 --- a/packages/grpc-js/test/test-retry.ts +++ b/packages/grpc-js/test/test-retry.ts @@ -333,6 +333,7 @@ describe('Retries', () => { metadata, (error: grpc.ServiceError, response: any) => { assert(error); + assert.strictEqual(error.code, 300); assert.strictEqual(error.details, 'Failed on retry 0'); done(); } From b763f98e07a344db9358d3315f02bc05784c5b19 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 26 Jul 2024 14:50:13 -0700 Subject: [PATCH 06/12] grpc-js: Bump to 1.11.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 19c3aea87..73cbb2b95 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.11.1", + "version": "1.11.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 a6575c3e734876a0eae68cabee40fde8741c7f37 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 30 Jul 2024 17:59:08 -0700 Subject: [PATCH 07/12] grpc-js: Simplify pick_first behavior --- .../grpc-js/src/load-balancer-pick-first.ts | 66 ++++++++----------- 1 file changed, 28 insertions(+), 38 deletions(-) diff --git a/packages/grpc-js/src/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index e042e1161..933483fe8 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -320,21 +320,16 @@ export class PickFirstLoadBalancer implements LoadBalancer { private removeCurrentPick() { if (this.currentPick !== null) { - /* Unref can cause a state change, which can cause a change in the value - * of this.currentPick, so we hold a local reference to make sure that - * does not impact this function. */ - const currentPick = this.currentPick; - this.currentPick = null; - currentPick.unref(); - currentPick.removeConnectivityStateListener(this.subchannelStateListener); + this.currentPick.removeConnectivityStateListener(this.subchannelStateListener); this.channelControlHelper.removeChannelzChild( - currentPick.getChannelzRef() + this.currentPick.getChannelzRef() ); - if (this.reportHealthStatus) { - currentPick.removeHealthStateWatcher( - this.pickedSubchannelHealthListener - ); - } + this.currentPick.removeHealthStateWatcher( + this.pickedSubchannelHealthListener + ); + // Unref last, to avoid triggering listeners + this.currentPick.unref(); + this.currentPick = null; } } @@ -418,20 +413,25 @@ export class PickFirstLoadBalancer implements LoadBalancer { this.connectionDelayTimeout.unref?.(); } + /** + * Declare that the specified subchannel should be used to make requests. + * This functions the same independent of whether subchannel is a member of + * this.children and whether it is equal to this.currentPick. + * Prerequisite: subchannel.getConnectivityState() === READY. + * @param subchannel + */ private pickSubchannel(subchannel: SubchannelInterface) { - if (this.currentPick && subchannel.realSubchannelEquals(this.currentPick)) { - return; - } trace('Pick subchannel with address ' + subchannel.getAddress()); this.stickyTransientFailureMode = false; - this.removeCurrentPick(); - this.currentPick = subchannel; + /* Ref before removeCurrentPick and resetSubchannelList to avoid the + * refcount dropping to 0 during this process. */ subchannel.ref(); - if (this.reportHealthStatus) { - subchannel.addHealthStateWatcher(this.pickedSubchannelHealthListener); - } this.channelControlHelper.addChannelzChild(subchannel.getChannelzRef()); + this.removeCurrentPick(); this.resetSubchannelList(); + subchannel.addConnectivityStateListener(this.subchannelStateListener); + subchannel.addHealthStateWatcher(this.pickedSubchannelHealthListener); + this.currentPick = subchannel; clearTimeout(this.connectionDelayTimeout); this.calculateAndReportNewState(); } @@ -448,20 +448,11 @@ export class PickFirstLoadBalancer implements LoadBalancer { private resetSubchannelList() { for (const child of this.children) { - 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 - * always happens immediately after the subchannel is picked. */ - child.subchannel.removeConnectivityStateListener( - this.subchannelStateListener - ); - } + /* Always remoev the connectivity state listener. If the subchannel is + getting picked, it will be re-added then. */ + child.subchannel.removeConnectivityStateListener( + this.subchannelStateListener + ); /* Refs are counted independently for the children list and the * currentPick, so we call unref whether or not the child is the * currentPick. Channelz child references are also refcounted, so @@ -478,15 +469,13 @@ export class PickFirstLoadBalancer implements LoadBalancer { } private connectToAddressList(addressList: SubchannelAddress[]) { + trace('connectToAddressList([' + addressList.map(address => subchannelAddressToString(address)) + '])'); const newChildrenList = addressList.map(address => ({ subchannel: this.channelControlHelper.createSubchannel(address, {}), hasReportedTransientFailure: false, })); - trace('connectToAddressList([' + addressList.map(address => subchannelAddressToString(address)) + '])'); for (const { subchannel } of newChildrenList) { if (subchannel.getConnectivityState() === ConnectivityState.READY) { - this.channelControlHelper.addChannelzChild(subchannel.getChannelzRef()); - subchannel.addConnectivityStateListener(this.subchannelStateListener); this.pickSubchannel(subchannel); return; } @@ -522,6 +511,7 @@ export class PickFirstLoadBalancer implements LoadBalancer { if (!(lbConfig instanceof PickFirstLoadBalancingConfig)) { return; } + this.requestedResolutionSinceLastUpdate = false; /* 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. */ From d409311bf2e9cfa3e4dee1ac68086c18338d206e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 1 Aug 2024 15:39:13 -0700 Subject: [PATCH 08/12] grpc-js: Report session error events instead of waiting for close --- packages/grpc-js/src/transport.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 1acbab40e..b7d066d5e 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -223,9 +223,8 @@ class Http2Transport implements Transport { ); session.once('error', error => { - /* Do nothing here. Any error should also trigger a close event, which is - * where we want to handle that. */ this.trace('connection closed with error ' + (error as Error).message); + this.handleDisconnect(); }); if (logging.isTracerEnabled(TRACER_NAME)) { @@ -383,6 +382,9 @@ class Http2Transport implements Transport { * Handle connection drops, but not GOAWAYs. */ private handleDisconnect() { + if (this.disconnectHandled) { + return; + } this.clearKeepaliveTimeout(); this.reportDisconnectToOwner(false); /* Give calls an event loop cycle to finish naturally before reporting the @@ -768,6 +770,7 @@ export class Http2SubchannelConnector implements SubchannelConnector { ); this.session = session; let errorMessage = 'Failed to connect'; + let reportedError = false; session.unref(); session.once('connect', () => { session.removeAllListeners(); @@ -778,12 +781,19 @@ export class Http2SubchannelConnector implements SubchannelConnector { this.session = null; // Leave time for error event to happen before rejecting setImmediate(() => { - reject(`${errorMessage} (${new Date().toISOString()})`); + if (!reportedError) { + reportedError = true; + reject(`${errorMessage} (${new Date().toISOString()})`); + } }); }); session.once('error', error => { errorMessage = (error as Error).message; this.trace('connection failed with error ' + errorMessage); + if (!reportedError) { + reportedError = true; + reject(`${errorMessage} (${new Date().toISOString()})`); + } }); }); } From 3d43b1a7be9e370255f8c2a8e4350aeaff2a829c Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 2 Aug 2024 15:44:07 -0700 Subject: [PATCH 09/12] Further reduce pick_first state space --- .../grpc-js/src/load-balancer-pick-first.ts | 31 ++++--------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/packages/grpc-js/src/load-balancer-pick-first.ts b/packages/grpc-js/src/load-balancer-pick-first.ts index 933483fe8..145ca3e39 100644 --- a/packages/grpc-js/src/load-balancer-pick-first.ts +++ b/packages/grpc-js/src/load-balancer-pick-first.ts @@ -213,8 +213,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { */ private connectionDelayTimeout: NodeJS.Timeout; - private triedAllSubchannels = false; - /** * The LB policy enters sticky TRANSIENT_FAILURE mode when all * subchannels have failed to connect at least once, and it stays in that @@ -225,12 +223,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { private reportHealthStatus: boolean; - /** - * 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. @@ -259,6 +251,10 @@ export class PickFirstLoadBalancer implements LoadBalancer { return this.children.every(child => child.hasReportedTransientFailure); } + private resetChildrenReportedTF() { + this.children.every(child => child.hasReportedTransientFailure = false); + } + private calculateAndReportNewState() { if (this.currentPick) { if (this.reportHealthStatus && !this.currentPick.isHealthy()) { @@ -291,7 +287,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { } private requestReresolution() { - this.requestedResolutionSinceLastUpdate = true; this.channelControlHelper.requestReresolution(); } @@ -299,15 +294,8 @@ export class PickFirstLoadBalancer implements LoadBalancer { if (!this.allChildrenHaveReportedTF()) { return; } - 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(); - } + this.requestReresolution(); + this.resetChildrenReportedTF(); if (this.stickyTransientFailureMode) { return; } @@ -369,9 +357,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { private startNextSubchannelConnecting(startIndex: number) { clearTimeout(this.connectionDelayTimeout); - if (this.triedAllSubchannels) { - return; - } for (const [index, child] of this.children.entries()) { if (index >= startIndex) { const subchannelState = child.subchannel.getConnectivityState(); @@ -384,7 +369,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { } } } - this.triedAllSubchannels = true; this.maybeEnterStickyTransientFailureMode(); } @@ -464,8 +448,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { } this.currentSubchannelIndex = 0; this.children = []; - this.triedAllSubchannels = false; - this.requestedResolutionSinceLastUpdate = false; } private connectToAddressList(addressList: SubchannelAddress[]) { @@ -511,7 +493,6 @@ export class PickFirstLoadBalancer implements LoadBalancer { if (!(lbConfig instanceof PickFirstLoadBalancingConfig)) { return; } - this.requestedResolutionSinceLastUpdate = false; /* 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. */ From 90e472e4a139fc8cd6714dffca188e633013e381 Mon Sep 17 00:00:00 2001 From: Ygal Bellaiche Date: Thu, 5 Sep 2024 10:44:36 +0300 Subject: [PATCH 10/12] made the call to trace to be called only when tracer is enabled --- packages/grpc-js/src/internal-channel.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/grpc-js/src/internal-channel.ts b/packages/grpc-js/src/internal-channel.ts index 4de4ad374..67c94d89c 100644 --- a/packages/grpc-js/src/internal-channel.ts +++ b/packages/grpc-js/src/internal-channel.ts @@ -31,7 +31,7 @@ import { getDefaultAuthority, mapUriDefaultScheme, } from './resolver'; -import { trace } from './logging'; +import { trace, isTracerEnabled } from './logging'; import { SubchannelAddress } from './subchannel-address'; import { mapProxyName } from './http_proxy'; import { GrpcUri, parseUri, uriToString } from './uri-parser'; @@ -424,15 +424,17 @@ export class InternalChannel { JSON.stringify(options, undefined, 2) ); const error = new Error(); - trace( - LogVerbosity.DEBUG, - 'channel_stacktrace', - '(' + - this.channelzRef.id + - ') ' + - 'Channel constructed \n' + - error.stack?.substring(error.stack.indexOf('\n') + 1) - ); + if (isTracerEnabled('channel_stacktrace')){ + trace( + LogVerbosity.DEBUG, + 'channel_stacktrace', + '(' + + this.channelzRef.id + + ') ' + + 'Channel constructed \n' + + error.stack?.substring(error.stack.indexOf('\n') + 1) + ); + } this.lastActivityTimestamp = new Date(); } From b3c24d080f4623952ac2d693b85e567ad51c78d2 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 12 Sep 2024 14:31:01 -0700 Subject: [PATCH 11/12] grpc-js: round_robin: Request resolution on connection drop --- packages/grpc-js/package.json | 2 +- packages/grpc-js/src/load-balancer-round-robin.ts | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 73cbb2b95..5cb8796e8 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.11.2", + "version": "1.11.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/load-balancer-round-robin.ts b/packages/grpc-js/src/load-balancer-round-robin.ts index 7e70c554f..6e2b0b11d 100644 --- a/packages/grpc-js/src/load-balancer-round-robin.ts +++ b/packages/grpc-js/src/load-balancer-round-robin.ts @@ -110,6 +110,13 @@ export class RoundRobinLoadBalancer implements LoadBalancer { channelControlHelper, { updateState: (connectivityState, picker) => { + /* Ensure that name resolution is requested again after active + * connections are dropped. This is more aggressive than necessary to + * accomplish that, so we are counting on resolvers to have + * reasonable rate limits. */ + if (this.currentState === ConnectivityState.READY && connectivityState !== ConnectivityState.READY) { + this.channelControlHelper.requestReresolution(); + } this.calculateAndUpdateState(); }, } From 65cd9b678e65ca1bd3b8ce2d368cbac61adc00f4 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 8 Oct 2024 09:24:58 -0700 Subject: [PATCH 12/12] grpc-js: Bump to 1.12.1 --- 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 79ae5e40e..ffa8539c7 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.12.0", + "version": "1.12.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",