feat: Improve handling of HTTP/HTTPS agent sessions by davidkna-sap · Pull Request #6498 · SAP/cloud-sdk-js

davidkna-sap · 2026-04-14T11:43:37Z

Closes https://github.com/SAP/cloud-sdk-backlog/issues/1298

For non-custom http/https agent default to node's global http/https agents (default: keepAlive === true)
Cache custom HTTP/HTTPS agents so identical agent configurations are reused instead of creating new agents repeatedly.
Bound the agent cache with new LRU eviction to limit memory growth due to caching, since unbounded agent creation lead to noticeable memory consumption.
Add getOrInsertComputed() to Cache<_> (like Map.prototype.getOrInsertComputed())

Co-authored-by: David Knaack <[email protected]>

ZhongpinWang

Overall very nice implementation. 👍 I love the idea of making our cache a LRU cache. Btw we also have an async cache, not sure if that one needs to be changed as well. Otherwise, some small suggestions.

ZhongpinWang · 2026-04-14T13:57:56Z

@@ -82,10 +102,41 @@ export class Cache<T> implements CacheInterface<T> {
   * @param item - The entry to cache.
   */
  set(key: string | undefined, item: CacheEntry<T>): void {


[q] In general, I wonder why this key can be undefined :)

Claude is claiming it's for the benefit of getDestinationCacheKey

ZhongpinWang · 2026-04-14T14:11:04Z

   * Object that stores all cached entries.
   */
-  private cache: Record<string, CacheEntry<T>>;
+  private cache: Map<string, CacheEntry<T>>;


Nice 👍 Map maintains the insertion order.

ZhongpinWang · 2026-04-14T14:17:31Z

+ * @returns A hash of the given value using a cryptographic hash function.
+ */
+export function hashCacheKey(value: Record<string, unknown>): string {
+  const serialized = JSON.stringify(value);


Have you checked if this is sufficient? JSON.stringify cannot stringify many things like Map or Set etc.

Maybe check alternatives https://www.npmjs.com/package/fast-json-stable-stringify or something else.

I switched to ohash (v1 instead of v2 for CJS support, v1 is still getting updates), Map/Set wouldn't be an issue here, but the values can contain functions.

I can switch to JSON.stringify's replacer option to handle functions if you prefer that, as TS is also having some issues with ohash in general.

I also did some search, but tbh none of them seems to be very actively maintained. Maybe they just work without problem.

Also maybe we can try require() now since node 20 is (very soon) EOL

This reverts commit 002f644.

marikaner

Didn't get through everything yet. Seems like an improvement 😊

marikaner · 2026-04-15T12:15:16Z

+    const entry = key ? this.cache.get(key) : undefined;
+    if (entry) {
+      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.maxSize !== Infinity) {
+        this.cache.delete(key!);
+        this.cache.set(key!, entry);
+      }
+    }
+    return entry?.entry;


[pp]

Suggested change

const entry = key ? this.cache.get(key) : undefined;

if (entry) {

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.maxSize !== Infinity) {

this.cache.delete(key!);

this.cache.set(key!, entry);

}

}

return entry?.entry;

if (key) {

const entry = this.cache.get(key);

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.maxSize !== Infinity) {

this.cache.delete(key);

this.cache.set(key, entry);

}

return entry.entry;

}

Handled this differently because this.cache.get(key); can return undefined.

Co-authored-by: Marika Marszalkowski <[email protected]> Co-authored-by: David Knaack <[email protected]>

* origin/main: (27 commits) chore: Replace mock-fs with memfs/unionfs for fs mocking (#6470) chore(deps-dev): bump @changesets/cli from 2.30.0 to 2.31.0 (#6515) chore(deps): bump bignumber.js from 10.0.2 to 11.0.0 (#6511) chore(deps): bump @changesets/get-release-plan from 4.0.15 to 4.0.16 (#6518) chore(check-public-api): Use tempdir instead of mockfs (#6468) chore(deps): bump prettier from 3.8.2 to 3.8.3 (#6510) chore: Refactor test imports to use @sap-cloud-sdk/test-util-internal pkg (#6467) chore: Add license-checker action for pnpm (#6473) chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495) chore: Add composite setup action for pnpm (#6507) fix: Avoid caching jwt if it needs to be forwarded (#6007) Change status from proposed to decided chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514) chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513) chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512) chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509) chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508) chore(deps-dev): bump typedoc from 0.28.18 to 0.28.19 (#6505) chore(deps): bump ts-morph from 27.0.2 to 28.0.0 (#6506) chore(deps-dev): bump globals from 17.4.0 to 17.5.0 (#6504) ...

marikaner

I left a few comments, please especially take note of the ones with "[req]". But no big blockers.

marikaner · 2026-04-22T08:54:30Z

+      this.cache.delete(key!);
+      this.cache.set(key!, entry);


[q] is the ! needed? There is a nullish check on the top.

It was needed before the refactoring, I was able to remove them now.

marikaner · 2026-04-22T09:29:44Z

+    ) {
+      // Evict the least recently used (LRU) entry
+      const lruKey = this.cache.keys().next().value;
+      this.cache.delete(lruKey!); // SAFETY: size > 0


[q] What does the comment tell me?

The comment explains why the ! operator is safe to apply here.

marikaner · 2026-04-22T09:50:47Z

+  ): T {
+    if (!key) {
+      return computeFn().entry;
+    }


[req] I wasn't sure about this, because it seems a little off: It is weird, that this just calls the compute function without caching it. But, when you have a case where the key might not be defined and you want to have a default you don't have to differentiate the cases. So I guess it is better as is.

But, I am missing signs that this is intended, though, aka. a test and docs.

I've removed this special handling, I added in on purpose but is currently not a useful feature in practice.

* origin/main: chore: Replace yarn with pnpm (#6448) chore: Reorder step configurations (#6521) chore(deps): bump bignumber.js from 10.0.2 to 11.0.0 (#6520) chore(deps): bump fast-xml-parser from 5.6.0 to 5.7.1 (#6519) feat: Add `getIasToken()` and `getIasDestination()` convenience functions (#6431) chore(deps-dev): bump memfs from 4.57.1 to 4.57.2 (#6516) chore: Correct merge-and-write-changelogs action path in package.json (#6479) chore(deps-dev): bump turbo from 2.8.10 to 2.9.6 (#6499)

feat: Improve handling of HTTP/HTTPS agent sessions

68fd5d8

davidkna-sap force-pushed the davidkna-sap_cache-agent branch from a1f20f3 to 68fd5d8 Compare April 14, 2026 12:03

davidkna-sap and others added 2 commits April 14, 2026 14:27

chore: Hash cache-key to shorten it

fdbad2c

Changes from lint:fix

ea8a2a2

davidkna-sap commented Apr 14, 2026

View reviewed changes

Comment thread .changeset/heavy-foxes-deny.md Outdated

Apply suggestions from code review

6fb4d87

Co-authored-by: David Knaack <[email protected]>

davidkna-sap commented Apr 14, 2026

View reviewed changes

Comment thread packages/connectivity/src/http-agent/http-agent.ts

Apply suggestion from @davidkna-sap

8ca6638

davidkna-sap marked this pull request as ready for review April 14, 2026 13:20

davidkna-sap requested review from KavithaSiva, ZhongpinWang and marikaner April 14, 2026 13:20

ZhongpinWang reviewed Apr 14, 2026

View reviewed changes

davidkna-sap force-pushed the davidkna-sap_cache-agent branch from cdf9b3a to e535ea9 Compare April 14, 2026 15:12

davidkna-sap requested a review from ZhongpinWang April 14, 2026 15:15

Use ohash for cache key generation

002f644

davidkna-sap force-pushed the davidkna-sap_cache-agent branch from e535ea9 to 002f644 Compare April 14, 2026 15:17

davidkna-sap added 2 commits April 15, 2026 13:17

Revert "Use ohash for cache key generation"

b784961

This reverts commit 002f644.

refactor: Use JSON.stringify replacer

7af67b9

marikaner reviewed Apr 15, 2026

View reviewed changes

davidkna-sap commented Apr 15, 2026

View reviewed changes

Comment thread packages/connectivity/src/http-agent/http-agent.ts Outdated

davidkna-sap and others added 3 commits April 15, 2026 17:15

feat: Add safe-stable-stringify for deterministic object serialization

341b309

Apply suggestions from code review

2c33adf

Co-authored-by: Marika Marszalkowski <[email protected]> Co-authored-by: David Knaack <[email protected]>

chore: fix import

911cbb7

davidkna-sap requested a review from marikaner April 16, 2026 09:28

davidkna-sap added 2 commits April 17, 2026 12:16

feat: Update hashCacheKey to use crypto.hash for Node.js v20.12.0+

1e9e397

marikaner approved these changes Apr 22, 2026

View reviewed changes

address review feedback

b68bf7f

davidkna-sap requested a review from marikaner April 22, 2026 11:03

Conversation

davidkna-sap commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZhongpinWang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhongpinWang Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marikaner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marikaner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkna-sap Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidkna-sap commented Apr 14, 2026 •

edited

Loading

ZhongpinWang Apr 15, 2026 •

edited

Loading

davidkna-sap Apr 22, 2026 •

edited

Loading