refactor(sdkv3): cache clients on creation.

[Hweinstock]

Problem

The toolkit recreates each SDK client before each request is made. As an example see:

aws-toolkit-vscode/packages/core/src/shared/clients/ec2Client.ts

Lines 31 to 52 in b4d7417

	export class Ec2Client {
	public constructor(public readonly regionCode: string) {}
	private async createSdkClient(): Promise<EC2> {
	return await globals.sdkClientBuilder.createAwsService(EC2, undefined, this.regionCode)
	}
	public async getInstances(filters?: EC2.Filter[]): Promise<AsyncCollection<SafeEc2Instance>> {
	const client = await this.createSdkClient()
	const requester = async (request: EC2.DescribeInstancesRequest) => client.describeInstances(request).promise()
	const collection = pageableToCollection(
	requester,
	filters ? { Filters: filters } : {},
	'NextToken',
	'Reservations'
	)
	const extractedInstances = this.getInstancesFromReservations(collection)
	const instances = await this.updateInstancesDetail(extractedInstances)
	return instances
	}

This is done as an unnecessary guard against the client's internal state interfering with higher level business logic. Creating clients for each request is not cheap, and there is likely a better pattern to be had here.

See internal docs expanding more on this issue.

Solution

• Create a map that caches the clients.
• Key the function arguments such that it detects config changes.
• Cache clients with new method getAwsService that wraps createAwsService.
• Allow consumers to pass a ignoreCache flag to force creation of a new client.
• On deactivation, call destroy on all the clients.

Testing Strategy

• Create clients with methods that make it easy to determine equality.

---

• 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.
• License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

[justinmk3]

can we extend the existing memoize()? also probably want direct test coverage of this

[Hweinstock]

I was trying to maintain its type signature. Making this one return an async method would cause a lot of unrelated changes. However, no longer relevant since I refactored away from this.

[justinmk3]

Making this one return an async method

would an "overload" work?

[justinmk3]

memoize may not be gaining much for this use-case.

because this is an important core module, we may want more visibility into the long-lived container that stores the clients. this module could store them in a map, that could be inspected and/or logged.

[Hweinstock]

That makes sense, this will make it easier to add custom-logic later. Implementing it this way also makes it seem like a natural place to ensure all clients call destroy on deactivation.

[Hweinstock]

/runIntegrationTests

[justinmk3]

Is JSON.stringify guaranteed to return a stable ordering of keys for the same input? I think we have some "hash" code, or some other way of dealing with that, somewhere...

[justinmk3]

should the docstring call out the fact that the client will be re-used?

important for callers not to "modify" the client, except in specific "safe" ways...

is it possible that a cached client is modified such that its key no longer semantically matches its value? e.g. could its region be changed, but the "key" still has the old region?