From 2086649d8a1e85b28a51bc8ca81513b7101910f3 Mon Sep 17 00:00:00 2001 From: Hweinstock <42325418+Hweinstock@users.noreply.github.com> Date: Mon, 3 Mar 2025 11:38:51 -0500 Subject: [PATCH] refactor(http): inject same http handler into each sdk client. (#6690) ## Problem From https://github.com/aws/aws-toolkit-vscode/pull/6664, we have persistent connections. However, each SDK client creates its own http handler. If we have N distinct service clients we maintain, then we could have up to N http handlers, with N distinct connection pools. This does not affect persistent connections since each service maintains its own endpoints, however, there is a small overhead in initiating each connection pool. Additionally, there is no guarantee for consistent behavior across handlers with regards to configuration options (Ex. requestTimeout). ## Solution - inject the same HTTP handler into each SDK client, unless explicitly given a different one. - use fetch-http-handler on web and `node-http-handler` on node. - We don't want to use `fetch-http-handler` in node because it still has experimental support and is not recommended. [docs](https://github.com/aws/aws-sdk-js-v3/blob/main/supplemental-docs/CLIENTS.md#request-handler-requesthandler) and [comment](https://github.com/aws/aws-sdk-js-v3/issues/4619#issuecomment-2110481501) from SDK team. When trying to, there were issues with persistent connections. - install `http2` to resolve web deps issue. This is part of nodes standard library, but needed as explicit dependency for web. ### Trying `fetch-http-handler` in node. - confirmed `fetch-http-handler` with `keep-alive: true` option is sending `keepAlive` headers, but closing the connection after doing so in node, both in VSCode environment, and outside of it in a pure node environment. This implies it is not related to https://github.com/microsoft/vscode/issues/173861. ## Verification The request times seemed unaffected by this change, but there was a noticeable impact on sdk client initialization speed. The results below are from creating 1000 SSM clients with and without the same HTTP Handler. image Because we usually cache the SDK clients under each service, the important statistic is that this speeds up 0.131 ms per SDK client creation. If we always use the cache and only create a client once per service, then this also suggests a 0.131 ms per service speedup. We interact with at least 20 services, and 16 in the explorer alone, so this could result in 2-2.5 ms improvement in initialization time for all these SDK clients depending on how they are created. Could be interesting to revisit after the migration to see if this reduces start-up time. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --- package-lock.json | 104 ++++++++++++++++++ packages/core/package.json | 5 +- .../core/src/shared/awsClientBuilderV3.ts | 21 ++++ .../test/shared/awsClientBuilderV3.test.ts | 33 ++++++ 4 files changed, 162 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 64df6e937a..0d12006ff8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,6 +16,7 @@ ], "dependencies": { "@types/node": "^22.7.5", + "http2": "^3.3.6", "vscode-nls": "^5.2.0", "vscode-nls-dev": "^4.0.4" }, @@ -10400,6 +10401,16 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/http2": { + "version": "3.3.6", + "resolved": "https://registry.npmjs.org/http2/-/http2-3.3.6.tgz", + "integrity": "sha512-L8iE1xmQ9hZxalL9ekcx6Dp98h8Fwfxnv9Kq8cf7adrXmyYGBnkbQeJe7MBHPkXxjITIOM1R7XWQYo4iDaNAuQ==", + "deprecated": "Use the built-in module in node 9.0.0 or newer, instead", + "license": "MIT", + "engines": { + "node": ">=0.12.0" + } + }, "node_modules/http2-wrapper": { "version": "1.0.3", "license": "MIT", @@ -17130,7 +17141,9 @@ "@aws/mynah-ui": "^4.22.1", "@gerhobbelt/gitignore-parser": "^0.2.0-9", "@iarna/toml": "^2.2.5", + "@smithy/fetch-http-handler": "^3.0.0", "@smithy/middleware-retry": "^3.0.0", + "@smithy/node-http-handler": "^3.0.0", "@smithy/protocol-http": "^4.0.0", "@smithy/service-error-classification": "^3.0.0", "@smithy/shared-ini-file-loader": "^3.0.0", @@ -17340,6 +17353,32 @@ "node": ">=16.0.0" } }, + "packages/core/node_modules/@aws-sdk/client-docdb-elastic/node_modules/@smithy/fetch-http-handler": { + "version": "4.1.3", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-4.1.3.tgz", + "integrity": "sha512-6SxNltSncI8s689nvnzZQc/dPXcpHQ34KUj6gR/HBroytKOd/isMG3gJF/zBE1TBmTT18TXyzhg3O3SOOqGEhA==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.8", + "@smithy/querystring-builder": "^3.0.11", + "@smithy/types": "^3.7.2", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, + "packages/core/node_modules/@aws-sdk/client-docdb/node_modules/@smithy/fetch-http-handler": { + "version": "4.1.3", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-4.1.3.tgz", + "integrity": "sha512-6SxNltSncI8s689nvnzZQc/dPXcpHQ34KUj6gR/HBroytKOd/isMG3gJF/zBE1TBmTT18TXyzhg3O3SOOqGEhA==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.8", + "@smithy/querystring-builder": "^3.0.11", + "@smithy/types": "^3.7.2", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, "packages/core/node_modules/@aws-sdk/client-sso": { "version": "3.693.0", "license": "Apache-2.0", @@ -17438,6 +17477,32 @@ "@aws-sdk/client-sts": "^3.693.0" } }, + "packages/core/node_modules/@aws-sdk/client-sso-oidc/node_modules/@smithy/fetch-http-handler": { + "version": "4.1.3", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-4.1.3.tgz", + "integrity": "sha512-6SxNltSncI8s689nvnzZQc/dPXcpHQ34KUj6gR/HBroytKOd/isMG3gJF/zBE1TBmTT18TXyzhg3O3SOOqGEhA==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.8", + "@smithy/querystring-builder": "^3.0.11", + "@smithy/types": "^3.7.2", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, + "packages/core/node_modules/@aws-sdk/client-sso/node_modules/@smithy/fetch-http-handler": { + "version": "4.1.3", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-4.1.3.tgz", + "integrity": "sha512-6SxNltSncI8s689nvnzZQc/dPXcpHQ34KUj6gR/HBroytKOd/isMG3gJF/zBE1TBmTT18TXyzhg3O3SOOqGEhA==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.8", + "@smithy/querystring-builder": "^3.0.11", + "@smithy/types": "^3.7.2", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, "packages/core/node_modules/@aws-sdk/client-sts": { "version": "3.693.0", "license": "Apache-2.0", @@ -17487,6 +17552,19 @@ "node": ">=16.0.0" } }, + "packages/core/node_modules/@aws-sdk/client-sts/node_modules/@smithy/fetch-http-handler": { + "version": "4.1.3", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-4.1.3.tgz", + "integrity": "sha512-6SxNltSncI8s689nvnzZQc/dPXcpHQ34KUj6gR/HBroytKOd/isMG3gJF/zBE1TBmTT18TXyzhg3O3SOOqGEhA==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.8", + "@smithy/querystring-builder": "^3.0.11", + "@smithy/types": "^3.7.2", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, "packages/core/node_modules/@aws-sdk/core": { "version": "3.693.0", "license": "Apache-2.0", @@ -17526,6 +17604,19 @@ "node": ">=16.0.0" } }, + "packages/core/node_modules/@aws-sdk/credential-provider-http/node_modules/@smithy/fetch-http-handler": { + "version": "4.1.3", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-4.1.3.tgz", + "integrity": "sha512-6SxNltSncI8s689nvnzZQc/dPXcpHQ34KUj6gR/HBroytKOd/isMG3gJF/zBE1TBmTT18TXyzhg3O3SOOqGEhA==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.8", + "@smithy/querystring-builder": "^3.0.11", + "@smithy/types": "^3.7.2", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, "packages/core/node_modules/@aws-sdk/credential-provider-ini": { "version": "3.693.0", "license": "Apache-2.0", @@ -17752,6 +17843,19 @@ } } }, + "packages/core/node_modules/@smithy/fetch-http-handler": { + "version": "3.2.9", + "resolved": "https://registry.npmjs.org/@smithy/fetch-http-handler/-/fetch-http-handler-3.2.9.tgz", + "integrity": "sha512-hYNVQOqhFQ6vOpenifFME546f0GfJn2OiQ3M0FDmuUu8V/Uiwy2wej7ZXxFBNqdx0R5DZAqWM1l6VRhGz8oE6A==", + "license": "Apache-2.0", + "dependencies": { + "@smithy/protocol-http": "^4.1.4", + "@smithy/querystring-builder": "^3.0.7", + "@smithy/types": "^3.5.0", + "@smithy/util-base64": "^3.0.0", + "tslib": "^2.6.2" + } + }, "packages/core/node_modules/@smithy/is-array-buffer": { "version": "3.0.0", "license": "Apache-2.0", diff --git a/packages/core/package.json b/packages/core/package.json index 890e1e7300..a45bc88229 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -521,6 +521,8 @@ "@smithy/service-error-classification": "^3.0.0", "@smithy/shared-ini-file-loader": "^3.0.0", "@smithy/util-retry": "^3.0.0", + "@smithy/fetch-http-handler": "^3.0.0", + "@smithy/node-http-handler": "^3.0.0", "@vscode/debugprotocol": "^1.57.0", "@zip.js/zip.js": "^2.7.41", "adm-zip": "^0.5.10", @@ -560,7 +562,8 @@ "winston": "^3.11.0", "winston-transport": "^4.6.0", "xml2js": "^0.6.1", - "yaml-cfn": "^0.3.2" + "yaml-cfn": "^0.3.2", + "http2": "^3.3.6" }, "overrides": { "webfont": { diff --git a/packages/core/src/shared/awsClientBuilderV3.ts b/packages/core/src/shared/awsClientBuilderV3.ts index 54dfc40b0f..1f117072f6 100644 --- a/packages/core/src/shared/awsClientBuilderV3.ts +++ b/packages/core/src/shared/awsClientBuilderV3.ts @@ -25,6 +25,7 @@ import { RetryStrategy, UserAgent, } from '@aws-sdk/types' +import { FetchHttpHandler } from '@smithy/fetch-http-handler' import { HttpResponse, HttpRequest } from '@aws-sdk/protocol-http' import { ConfiguredRetryStrategy } from '@smithy/util-retry' import { telemetry } from './telemetry/telemetry' @@ -33,6 +34,8 @@ import { extensionVersion } from './vscode/env' import { getLogger } from './logger/logger' import { partialClone } from './utilities/collectionUtils' import { selectFrom } from './utilities/tsUtils' +import { once } from './utilities/functionUtils' +import { isWeb } from './extensionGlobals' export type AwsClientConstructor = new (o: AwsClientOptions) => C @@ -88,6 +91,20 @@ export class AWSClientBuilderV3 { return shim } + private buildHttpHandler() { + const requestTimeout = 30000 + // HACK: avoid importing node-http-handler on web. + return isWeb() + ? new FetchHttpHandler({ keepAlive: true, requestTimeout }) + : new (require('@smithy/node-http-handler').NodeHttpHandler)({ + httpAgent: { keepAlive: true }, + httpsAgent: { keepAlive: true }, + requestTimeout, + }) + } + + private getHttpHandler = once(this.buildHttpHandler.bind(this)) + private keyAwsService(serviceOptions: AwsServiceOptions): string { // Serializing certain objects in the args allows us to detect when nested objects change (ex. new retry strategy, endpoints) return [ @@ -129,6 +146,10 @@ export class AWSClientBuilderV3 { // Simple exponential backoff strategy as default. opt.retryStrategy = new ConfiguredRetryStrategy(5, (attempt: number) => 1000 * 2 ** attempt) } + + if (!opt.requestHandler) { + opt.requestHandler = this.getHttpHandler() + } // TODO: add tests for refresh logic. opt.credentials = async () => { const creds = await shim.get() diff --git a/packages/core/src/test/shared/awsClientBuilderV3.test.ts b/packages/core/src/test/shared/awsClientBuilderV3.test.ts index 440e87dac6..7cf76264c6 100644 --- a/packages/core/src/test/shared/awsClientBuilderV3.test.ts +++ b/packages/core/src/test/shared/awsClientBuilderV3.test.ts @@ -32,6 +32,7 @@ import { Credentials, MetadataBearer, MiddlewareStack } from '@aws-sdk/types' import { oneDay } from '../../shared/datetime' import { ConfiguredRetryStrategy } from '@smithy/util-retry' import { StandardRetryStrategy } from '@smithy/util-retry' +import { NodeHttpHandler } from '@smithy/node-http-handler' describe('AwsClientBuilderV3', function () { let builder: AWSClientBuilderV3 @@ -77,6 +78,38 @@ describe('AwsClientBuilderV3', function () { assert.strictEqual(service.config.userAgent[0][0], 'CUSTOM USER AGENT') }) + it('injects http client into handler', function () { + const requestHandler = new NodeHttpHandler({ + requestTimeout: 1234, + }) + const service = builder.createAwsService({ + serviceClient: Client, + clientOptions: { + requestHandler: requestHandler, + }, + }) + assert.strictEqual(service.config.requestHandler, requestHandler) + }) + + it('defaults to reusing singular http handler', function () { + const service = builder.createAwsService({ + serviceClient: Client, + }) + const service2 = builder.createAwsService({ + serviceClient: Client, + }) + + const firstHandler = service.config.requestHandler + const secondHandler = service2.config.requestHandler + + // If not injected, the http handler can be undefined before making request. + if (firstHandler instanceof NodeHttpHandler && secondHandler instanceof NodeHttpHandler) { + assert.ok(firstHandler === secondHandler) + } else { + assert.fail('Expected both request handlers to be NodeHttpHandler instances') + } + }) + describe('caching mechanism', function () { it('avoids recreating client on duplicate calls', async function () { const firstClient = builder.getAwsService({ serviceClient: TestClient })