Skip to content

Commit

Permalink
refactor(http): inject same http handler into each sdk client. (#6690)
Browse files Browse the repository at this point in the history
## Problem
From #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](aws/aws-sdk-js-v3#4619 (comment))
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
microsoft/vscode#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.
<img width="304" alt="image"
src="https://github.com/user-attachments/assets/9b28af43-795c-4dcb-9bb1-752c118a3247"
/>

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.
  • Loading branch information
Hweinstock authored Mar 3, 2025
1 parent eaadd25 commit 2086649
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 1 deletion.
104 changes: 104 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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": {
Expand Down
21 changes: 21 additions & 0 deletions packages/core/src/shared/awsClientBuilderV3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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<C> = new (o: AwsClientOptions) => C

Expand Down Expand Up @@ -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<C extends AwsClient>(serviceOptions: AwsServiceOptions<C>): string {
// Serializing certain objects in the args allows us to detect when nested objects change (ex. new retry strategy, endpoints)
return [
Expand Down Expand Up @@ -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()
Expand Down
33 changes: 33 additions & 0 deletions packages/core/src/test/shared/awsClientBuilderV3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 })
Expand Down

0 comments on commit 2086649

Please sign in to comment.