diff --git a/.changeset/heavy-foxes-deny.md b/.changeset/heavy-foxes-deny.md new file mode 100644 index 0000000000..c6a89a4690 --- /dev/null +++ b/.changeset/heavy-foxes-deny.md @@ -0,0 +1,6 @@ +--- +'@sap-cloud-sdk/connectivity': minor +'@sap-cloud-sdk/http-client': minor +--- + +[Improvement] Cache custom http and https agents and enable the keep-alive option by default. diff --git a/.changeset/swift-candies-bake.md b/.changeset/swift-candies-bake.md new file mode 100644 index 0000000000..025f1801a2 --- /dev/null +++ b/.changeset/swift-candies-bake.md @@ -0,0 +1,6 @@ +--- +'@sap-cloud-sdk/connectivity': minor +'@sap-cloud-sdk/http-client': minor +--- + +[Improvement] Use node's global http/https agent unless a custom agent is required by the destination configuration. \ No newline at end of file diff --git a/packages/connectivity/package.json b/packages/connectivity/package.json index 25e240706e..c24ff39a57 100644 --- a/packages/connectivity/package.json +++ b/packages/connectivity/package.json @@ -47,7 +47,8 @@ "async-retry": "^1.3.3", "axios": "^1.15.0", "jks-js": "^1.1.6", - "jsonwebtoken": "^9.0.3" + "jsonwebtoken": "^9.0.3", + "safe-stable-stringify": "^2.5.0" }, "devDependencies": { "@jest/globals": "^30.3.0", diff --git a/packages/connectivity/src/http-agent/http-agent.spec.ts b/packages/connectivity/src/http-agent/http-agent.spec.ts index 53ad664181..0ff8295115 100644 --- a/packages/connectivity/src/http-agent/http-agent.spec.ts +++ b/packages/connectivity/src/http-agent/http-agent.spec.ts @@ -15,7 +15,7 @@ jest.mock('jks-js', () => ({ import * as jks from 'jks-js'; import { certAsString } from '@sap-cloud-sdk/test-util-internal/test-certificate'; import { registerDestinationCache } from '../scp-cf/destination/register-destination-cache'; -import { getAgentConfig } from './http-agent'; +import { agentCache, getAgentConfig } from './http-agent'; import type { HttpDestination } from '../scp-cf/destination'; import type { DestinationCertificate } from '../scp-cf'; @@ -263,6 +263,53 @@ describe('createAgent', () => { // Check coverage }); +describe('agent caching', () => { + beforeEach(() => { + agentCache.clear(); + }); + + it('returns the same agent instance for the same destination options', async () => { + const destination: HttpDestination = { url: 'https://example.com' }; + const first = (await getAgentConfig(destination)) as any; + const second = (await getAgentConfig(destination)) as any; + expect(first['httpsAgent']).toBe(second['httpsAgent']); + }); + + it('returns different agent instances for different destinations', async () => { + const destA: HttpDestination = { url: 'https://a.example.com' }; + const destB: HttpDestination = { + url: 'https://b.example.com', + isTrustingAllCertificates: true + }; + const agentA = ((await getAgentConfig(destA)) as any)['httpsAgent']; + const agentB = ((await getAgentConfig(destB)) as any)['httpsAgent']; + expect(agentA).not.toBe(agentB); + }); + + it('enables keepAlive by default on created agents', async () => { + const destination: HttpDestination = { url: 'https://example.com' }; + const agent = ((await getAgentConfig(destination)) as any)['httpsAgent']; + expect(agent.keepAlive).toBe(true); + }); + + it('keepAlive: false in options overrides the default (spread order)', () => { + // createAgent uses { keepAlive: true, ...options } + // verify that explicit keepAlive: false in options wins + const merged = { keepAlive: true, ...({ keepAlive: false } as any) }; + expect(merged.keepAlive).toBe(false); + }); + + it('http and https destinations use separate cache entries and agent types', async () => { + const destHttps: HttpDestination = { url: 'https://example.com' }; + const destHttp: HttpDestination = { url: 'http://example.com' }; + const httpsResult = (await getAgentConfig(destHttps)) as any; + const httpResult = (await getAgentConfig(destHttp)) as any; + expect(httpsResult['httpsAgent']).toBeDefined(); + expect(httpResult['httpAgent']).toBeDefined(); + expect(httpsResult['httpsAgent']).not.toBe(httpResult['httpAgent']); + }); +}); + describe('getAgentConfig', () => { it('returns an object with key "httpsAgent" for destinations with protocol HTTPS', async () => { const destination: HttpDestination = { diff --git a/packages/connectivity/src/http-agent/http-agent.ts b/packages/connectivity/src/http-agent/http-agent.ts index f9350c10d9..1ed454c9d8 100644 --- a/packages/connectivity/src/http-agent/http-agent.ts +++ b/packages/connectivity/src/http-agent/http-agent.ts @@ -1,11 +1,13 @@ -import { readFile } from 'fs/promises'; -import http from 'http'; -import https from 'https'; +import { readFile } from 'node:fs/promises'; +import http from 'node:http'; +import https from 'node:https'; import * as jks from 'jks-js'; import { createLogger, last } from '@sap-cloud-sdk/util'; /* Careful the proxy imports cause circular dependencies if imported from scp directly */ // eslint-disable-next-line import/no-internal-modules import { getProtocolOrDefault } from '../scp-cf/get-protocol'; +// eslint-disable-next-line import/no-internal-modules +import { Cache, hashCacheKey } from '../scp-cf/cache'; import { addProxyConfigurationInternet, getProxyConfig, @@ -15,11 +17,13 @@ import { // eslint-disable-next-line import/no-internal-modules import { registerDestinationCache } from '../scp-cf/destination/register-destination-cache'; import type { - BasicProxyConfiguration, Destination, DestinationCertificate, HttpDestination -} from '../scp-cf'; + // eslint-disable-next-line import/no-internal-modules +} from '../scp-cf/destination'; +// eslint-disable-next-line import/no-internal-modules +import type { BasicProxyConfiguration } from '../scp-cf/connectivity-service-types'; import type { HttpAgentConfig, HttpsAgentConfig } from './agent-config'; const logger = createLogger({ @@ -283,17 +287,40 @@ function validateFormat(certificate: DestinationCertificate) { } } +/** + * Cache for http(s) agents. + * Exported for testing purposes only. + * @internal + */ +export const agentCache = new Cache( + 3600000, // 1 hour + 100 // max 100 LRU-cached agents +); + /** * @internal + * Agents are cached for up to one hour, but can be evicted earlier if more than 100 agents are created. * See https://nodejs.org/api/https.html#https_https_createserver_options_requestlistener for details on the possible options */ function createAgent( destination: HttpDestination, options: https.AgentOptions ): HttpAgentConfig | HttpsAgentConfig { - return getProtocolOrDefault(destination) === 'https' - ? { httpsAgent: new https.Agent(options) } - : { httpAgent: new http.Agent(options) }; + const protocol = getProtocolOrDefault(destination); + const cacheKey = hashCacheKey({ protocol, options }); + + return agentCache.getOrInsertComputed(cacheKey, () => { + logger.debug( + `Creating new ${protocol.toUpperCase()} agent for destination ${destination.name || ''}` + ); + const optionsWithDefaults = { keepAlive: true, ...options }; + const entry = + protocol === 'https' + ? { httpsAgent: new https.Agent(optionsWithDefaults) } + : { httpAgent: new http.Agent(optionsWithDefaults) }; + + return { entry }; + }); } /** diff --git a/packages/connectivity/src/scp-cf/cache.spec.ts b/packages/connectivity/src/scp-cf/cache.spec.ts index d17d8d1562..9d40b7cb4f 100644 --- a/packages/connectivity/src/scp-cf/cache.spec.ts +++ b/packages/connectivity/src/scp-cf/cache.spec.ts @@ -1,4 +1,4 @@ -import { Cache } from './cache'; +import { Cache, hashCacheKey } from './cache'; import { clientCredentialsTokenCache } from './client-credentials-token-cache'; import { destinationCache } from './destination'; import type { AuthenticationType, Destination } from './destination'; @@ -97,4 +97,133 @@ describe('Cache', () => { const actual = cacheOne.get(undefined); expect(actual).toBeUndefined(); }); + + it('item should be retrievable multiple times (non-LRU cache)', () => { + cacheOne.set('multi', { entry: destinationOne }); + expect(cacheOne.get('multi')).toEqual(destinationOne); + expect(cacheOne.get('multi')).toEqual(destinationOne); + }); + + describe('LRU eviction (maxSize)', () => { + it('evicts the least recently used entry when maxSize is exceeded', () => { + const lruCache = new Cache(0, 2); + lruCache.set('a', { entry: 'A' }); + lruCache.set('b', { entry: 'B' }); + lruCache.set('c', { entry: 'C' }); // should evict 'a' + expect(lruCache.get('a')).toBeUndefined(); + expect(lruCache.get('b')).toEqual('B'); + expect(lruCache.get('c')).toEqual('C'); + }); + + it('updates LRU order on get, so recently accessed entry is not evicted', () => { + const lruCache = new Cache(0, 2); + lruCache.set('a', { entry: 'A' }); + lruCache.set('b', { entry: 'B' }); + lruCache.get('a'); // 'a' is now most recently used; 'b' becomes LRU + lruCache.set('c', { entry: 'C' }); // should evict 'b' + expect(lruCache.get('b')).toBeUndefined(); + expect(lruCache.get('a')).toEqual('A'); + expect(lruCache.get('c')).toEqual('C'); + }); + + it('repeated get keeps the accessed key as most recently used', () => { + const lruCache = new Cache(0, 2); + lruCache.set('a', { entry: 'A' }); + lruCache.set('b', { entry: 'B' }); + + lruCache.get('a'); + lruCache.get('a'); + lruCache.set('c', { entry: 'C' }); + + expect(lruCache.get('a')).toEqual('A'); + expect(lruCache.get('b')).toBeUndefined(); + expect(lruCache.get('c')).toEqual('C'); + }); + + it('does not evict another entry when updating an existing key', () => { + const lruCache = new Cache(0, 2); + lruCache.set('a', { entry: 'A' }); + lruCache.set('b', { entry: 'B' }); + + lruCache.set('b', { entry: 'B updated' }); + + expect(lruCache.get('a')).toEqual('A'); + expect(lruCache.get('b')).toEqual('B updated'); + }); + }); + + describe('hashCacheKey', () => { + it('produces the same hash for plain objects with different key insertion order', () => { + expect(hashCacheKey({ a: 1, b: 2 })).toEqual( + hashCacheKey({ b: 2, a: 1 }) + ); + }); + + it('produces different hashes for plain objects with different values', () => { + expect(hashCacheKey({ a: 1 })).not.toEqual(hashCacheKey({ a: 2 })); + }); + + it('produces the same hash for Maps with different insertion order', () => { + const m1 = new Map([ + ['x', 1], + ['y', 2] + ]); + const m2 = new Map([ + ['y', 2], + ['x', 1] + ]); + expect(hashCacheKey({ m: m1 })).toEqual(hashCacheKey({ m: m2 })); + }); + + it('produces the same hash for Sets with different insertion order', () => { + const s1 = new Set([1, 2, 3]); + const s2 = new Set([3, 1, 2]); + expect(hashCacheKey({ s: s1 })).toEqual(hashCacheKey({ s: s2 })); + }); + + it('produces the same hash for class instances regardless of property insertion order', () => { + class Point { + [key: string]: number; + } + const p1 = new Point(); + p1.y = 2; + p1.x = 1; + const p2 = new Point(); + p2.x = 1; + p2.y = 2; + expect(hashCacheKey({ p: p1 })).toEqual(hashCacheKey({ p: p2 })); + }); + it('preserves arrays as arrays (does not coerce to object)', () => { + expect(hashCacheKey({ arr: [1, 2, 3] })).not.toEqual( + hashCacheKey({ arr: { 0: 1, 1: 2, 2: 3 } }) + ); + }); + }); + + describe('getOrInsertComputed', () => { + it('returns cached value on subsequent calls without calling computeFn again', () => { + const compute = jest.fn().mockReturnValue({ entry: destinationOne }); + cacheOne.getOrInsertComputed('cached', compute); + const result = cacheOne.getOrInsertComputed('cached', compute); + expect(result).toEqual(destinationOne); + expect(compute).toHaveBeenCalledTimes(1); + const result2 = cacheOne.get('cached'); + expect(result2).toEqual(destinationOne); + }); + + it('stores a custom expiration returned by computeFn', () => { + jest.useFakeTimers(); + const timeToExpire = 5000; + const compute = jest.fn().mockReturnValue({ + entry: destinationOne, + expires: Date.now() + timeToExpire + }); + + cacheOne.getOrInsertComputed('custom-expire', compute); + jest.advanceTimersByTime(timeToExpire + 1); + + expect(cacheOne.get('custom-expire')).toBeUndefined(); + expect(compute).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/connectivity/src/scp-cf/cache.ts b/packages/connectivity/src/scp-cf/cache.ts index 88c42a540d..cd739e7ebf 100644 --- a/packages/connectivity/src/scp-cf/cache.ts +++ b/packages/connectivity/src/scp-cf/cache.ts @@ -1,8 +1,12 @@ +import * as crypto from 'node:crypto'; +import { stringify } from 'safe-stable-stringify'; + interface CacheInterface { hasKey(key: string): boolean; get(key: string | undefined): T | undefined; set(key: string | undefined, item: CacheEntry): void; clear(): void; + getOrInsertComputed(key: string, computeFn: () => CacheEntry): T; } /** @@ -39,21 +43,25 @@ export class Cache implements CacheInterface { /** * Object that stores all cached entries. */ - private cache: Record>; + private cache: Map>; /** * Creates an instance of Cache. * @param defaultValidityTime - The default validity time in milliseconds. Use 0 for unlimited cache duration. + * @param capacity - The maximum number of entries in the cache. Use Infinity for unlimited size. Items are evicted based on a least recently used (LRU) strategy. */ - constructor(private defaultValidityTime: number) { - this.cache = {}; + constructor( + private defaultValidityTime: number, + private capacity = Infinity + ) { + this.cache = new Map>(); } /** * Clear all cached items. */ clear(): void { - this.cache = {}; + this.cache.clear(); } /** @@ -62,7 +70,7 @@ export class Cache implements CacheInterface { * @returns A boolean value that indicates whether the entry exists in cache. */ hasKey(key: string): boolean { - return this.cache.hasOwnProperty(key); + return this.cache.has(key); } /** @@ -71,9 +79,25 @@ export class Cache implements CacheInterface { * @returns The corresponding entry to the provided key if it is still valid, returns `undefined` otherwise. */ get(key: string | undefined): T | undefined { - return key && this.hasKey(key) && !isExpired(this.cache[key]) - ? this.cache[key].entry - : undefined; + if (!key) { + return undefined; + } + const entry = this.cache.get(key); + if (!entry) { + return undefined; + } + + if (isExpired(entry)) { + this.cache.delete(key); + return undefined; + } + + // LRU cache: Move accessed entry to the end of the Map to mark it as recently used + if (this.capacity !== Infinity) { + this.cache.delete(key); + this.cache.set(key, entry); + } + return entry?.entry; } /** @@ -82,10 +106,35 @@ export class Cache implements CacheInterface { * @param item - The entry to cache. */ set(key: string | undefined, item: CacheEntry): void { - if (key) { - const expires = item.expires ?? this.inferDefaultExpirationTime(); - this.cache[key] = { entry: item.entry, expires }; + if (!key) { + return; + } + if ( + this.cache.size >= this.capacity && + this.cache.size && + !this.cache.has(key) + ) { + // Evict the least recently used (LRU) entry + const lruKey = this.cache.keys().next().value; + this.cache.delete(lruKey!); // SAFETY: size > 0 + } + if (this.capacity !== Infinity && this.cache.has(key)) { + // If the key already exists, delete it to update its position in the LRU order + this.cache.delete(key); + } + + const expires = item.expires ?? this.inferDefaultExpirationTime(); + this.cache.set(key, { entry: item.entry, expires }); + } + + getOrInsertComputed(key: string, computeFn: () => CacheEntry): T { + const cachedEntry = this.get(key); + if (cachedEntry !== undefined) { + return cachedEntry; } + const newEntry = computeFn(); + this.set(key, newEntry); + return newEntry.entry; } private inferDefaultExpirationTime(): number | undefined { @@ -98,6 +147,24 @@ export class Cache implements CacheInterface { } } +/** + * Hashes the given value to create a cache key. + * @internal + * @param value - The value to hash. + * @returns A hash of the given value using a cryptographic hash function. + */ +export function hashCacheKey(value: Record): string { + const serialized = stringify(value); + + // TODO: crypto.hash is available in Node.js v20.12.0 and later. + // Remove the fallback to crypto.createHash once Node.js 22 is the minimum supported version. + if (typeof crypto.hash === 'function') { + return crypto.hash('blake2s256', serialized, 'base64url'); + } + + return crypto.createHash('blake2s256').update(serialized).digest('base64url'); +} + function isExpired(item: CacheEntry): boolean { if (item.expires === undefined) { return false; diff --git a/packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts b/packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts index e0587239e1..9cbb2bbfcf 100644 --- a/packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts +++ b/packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts @@ -169,8 +169,8 @@ describe('destination cache', () => { jwt: providerUserToken, isolationStrategy: 'tenant-user' }); - const cacheKeys = Object.keys( - await (destinationCache.getCacheInstance() as any).cache.cache + const cacheKeys = Array.from( + (destinationCache.getCacheInstance() as any).cache.cache.keys() ); expect(cacheKeys[0]).toBe( getDestinationCacheKey( @@ -412,9 +412,7 @@ describe('destination cache', () => { expect(warn).toHaveBeenCalledWith( "Could not build destination cache key. Isolation strategy 'tenant' is used, but tenant ID is undefined in JWT." ); - expect( - Object.keys(destinationCache.getCacheInstance()['cache'].cache).length - ).toBe(0); + expect(destinationCache.getCacheInstance()['cache'].cache.size).toBe(0); }); it('ignores cache if isolation requires user JWT but the JWT is not provided', async () => { diff --git a/packages/http-client/src/http-client.ts b/packages/http-client/src/http-client.ts index 1e4e52da26..5b7f9ac78b 100644 --- a/packages/http-client/src/http-client.ts +++ b/packages/http-client/src/http-client.ts @@ -455,8 +455,8 @@ export function getAxiosConfigWithDefaultsWithoutMethod(): Omit< 'method' > { return { - httpAgent: new http.Agent(), - httpsAgent: new https.Agent(), + httpAgent: http.globalAgent, + httpsAgent: https.globalAgent, timeout: 0, // zero means no timeout https://github.com/axios/axios/blob/main/README.md#request-config paramsSerializer: { serialize: (params = {}) => diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3f965a5379..5e1c13d65b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -336,6 +336,9 @@ importers: jsonwebtoken: specifier: ^9.0.3 version: 9.0.3 + safe-stable-stringify: + specifier: ^2.5.0 + version: 2.5.0 devDependencies: '@jest/globals': specifier: ^30.3.0