Skip to content

feat(cache/unstable): add Cache - #7236

Open
tomas-zijdemans wants to merge 16 commits into
denoland:mainfrom
tomas-zijdemans:cache
Open

feat(cache/unstable): add Cache#7236
tomas-zijdemans wants to merge 16 commits into
denoland:mainfrom
tomas-zijdemans:cache

Conversation

@tomas-zijdemans

@tomas-zijdemans tomas-zijdemans commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Adds Cache<K, V>: one composition-based class covering LRU eviction, TTL expiration, stale-while-revalidate, and load-through (getOrLoad).

The old LruCache and TtlCache extend Map, so inherited methods bypass the eviction and expiry logic, and two separate subclasses mean LRU and TTL can't be combined. Cache owns a Map for storage and delegates deadline ordering to IndexedHeap, replacing per-entry setTimeouts with a single timer. Mode is determined by options, not class choice, and a discriminated union on CacheOptions makes illegal combinations compile-time errors.

New capabilities:

  • Combined LRU + TTL in a single cache
  • getOrLoad(key, loader) with automatic in-flight deduplication
  • Stale-while-revalidate with background refresh, error handling, and configurable soft/hard TTL thresholds
  • Per-entry TTL overrides and sliding expiration
  • Hit/miss/eviction stats, typed removal reasons, and Symbol.dispose

CacheLike replaces MemoizationCache as the structural cache type. MemoizationCache stays as a deprecated alias, so nothing is removed here.

Bonus: in my benchmarks, write-heavy workloads (set, eviction) are 4-22x faster than the old classes, and hot-key reads are 55x faster thanks to the linked list.

Changes since the review:

  • Purely additive now. The removal of LruCache/TtlCache moved to BREAKING(cache/unstable): remove LruCache and TtlCache #7265, as requested.
  • size counts only live entries and always agrees with the iterators.
  • Deleting the last pending deadline cancels the sweep timer (previously it stayed armed until the original deadline).
  • onRemove errors are contained where a throw would be uncatchable (timer sweep, refresh microtask). Synchronous paths still throw.
  • The sweep timer is unref'd, so an idle cache never keeps the process alive.
  • Class docs call out the name collision with the web Cache API global.

Depends on #7245: the heap import switches to @std/data-structures/indexed-heap once that lands. Merge order is #7265, then #7245, then this.

tomas-zijdemans and others added 11 commits April 22, 2026 14:13
Introduces a single `Cache` class that subsumes both `LruCache` and
`TtlCache` into a configuration-driven API (maxSize, ttl, sliding
expiration, stale-while-revalidate refresh, and onRemove). The new
implementation delegates expiration tracking to `IndexedHeap` from
`@std/data-structures/unstable-indexed-heap` for O(log n) evictions.

- Add cache/cache.ts and cache/cache_test.ts
- Remove cache/lru_cache.ts, cache/ttl_cache.ts and their tests
- Update cache/memoize.ts doc to reference Cache
- Update cache/mod.ts and cache/deno.json exports

Made-with: Cursor
@github-actions github-actions Bot added the cache label Jul 15, 2026
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.40476% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.07%. Comparing base (cdb83c6) to head (0fc1a85).

Files with missing lines Patch % Lines
cache/cache.ts 99.39% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7236      +/-   ##
==========================================
+ Coverage   95.03%   95.07%   +0.04%     
==========================================
  Files         618      619       +1     
  Lines       51596    52095     +499     
  Branches     9340     9465     +125     
==========================================
+ Hits        49035    49531     +496     
- Misses       2021     2022       +1     
- Partials      540      542       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bartlomieju

Copy link
Copy Markdown
Member

The engineering here is high quality and the motivation is sound — both LruCache and TtlCache extended Map, so inherited methods silently bypassed the eviction and expiry logic, and there was no way to compose the two. A single composition-based Cache is the right destination, and the test suite (2003 lines, FakeTime throughout, covering ttl: 0, negative/NaN validation, eviction ordering, re-entrancy, SWR races) is genuinely thorough.

My main request is about how this is packaged rather than what it does. As it stands, one PR removes LruCache, TtlCache and MemoizationCache, drops two export paths, and adds a large new API surface — getOrLoad with in-flight dedup, stale-while-revalidate, per-entry TTL and absolute-expiration overrides, stats, onRemove with typed reasons, Symbol.dispose, and a heap-driven timer replacing per-entry setTimeout. Reviewing the removals and the new surface together means neither gets the attention it deserves, and if something regresses we can't bisect to tell which half caused it.

Could you split the removals into their own commit ahead of the addition? Landing the deletion of the old classes separately, then adding Cache on top, makes each half reviewable on its own and gives us a clean revert point. The title also needs to reflect the removals — repo precedent for this is BREAKING(cache/unstable): (see 21ba810, 5fba5e0); feat(cache) understates a change that deletes three public symbols and two export entries.

Four things I'd want fixed regardless of how it's split:

  1. size disagrees with every other accessor (cache/cache.ts:481). Expired-but-unswept entries count toward size while has(), get() and the iterators all exclude them. It's documented, but size === 1 alongside [...cache].length === 0 is the kind of trap people hit in production and can't reproduce locally. Either sweep before reporting, or give it a name that doesn't imply agreement with iteration.

  2. Stale timer after removal (cache/cache.ts:543). #removeEntry deletes the key from the heap but never calls #scheduleTimer, and #scheduleTimer returns early on an empty heap without clearing #timerId. So deleting the last TTL entry leaves a live setTimeout holding the event loop open until the original deadline fires.

  3. #onTimer can throw from a timer callback (cache/cache.ts:609) when a user's onRemove throws. That's uncatchable and takes down the process. The clear() rethrow path is reachable from a refresh microtask with the same consequence.

  4. Timers aren't unref'd, so a TTL cache keeps the process alive unless explicitly disposed. This matches the old TtlCache behavior so it isn't a regression, but since we're redesigning the class anyway it's the right moment to decide deliberately.

One last naming thought: Cache shadows the global DOM Cache in a module marked browser compatible, so import { Cache } from "@std/cache" silently shadows the lib.dom type. Probably acceptable, but worth a conscious decision rather than discovering it later.

Note this also depends on #7245 landing, since the timer imports @std/data-structures/unstable-indexed-heap.

@tomas-zijdemans tomas-zijdemans changed the title feat(cache): add unified Cache feat(cache/unstable): add Cache Jul 31, 2026
@tomas-zijdemans

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. All four fixes are in, and the PR is restructured the way you asked:

  • The removals now live in BREAKING(cache/unstable): remove LruCache and TtlCache #7265, so this PR is purely additive (MemoizationCache stays as a deprecated alias of CacheLike). Retitled to feat(cache/unstable).
  • size now counts only live entries, so it always agrees with the iterators. I went with counting over sweeping so a property read never fires user callbacks.
  • Removing the last pending deadline (via delete() or an overwriting set()) now cancels the sweep timer, and the scheduler clears its state when the heap is empty.
  • onRemove errors are discarded in the two contexts where a throw is uncatchable (the timer sweep and the refresh microtask) and documented as such. Synchronous paths (delete(), clear(), expired reads) still propagate, matching what TtlCache did after BREAKING(cache/unstable): refactor TtlCache #7065.
  • Timers are unref'd unconditionally (same guarded pattern as async/delay.ts). A cache timer only exists to reclaim memory early, so it shouldn't keep a process alive.

On Cache shadowing the DOM global: I'd keep the name, as a conscious decision. The global is not constructible and shares no methods with this class, so a mixup fails loudly at compile time rather than silently. There's also in-repo precedent for a package's headline export taking a name a global holds: @std/crypto exports crypto, shadowing globalThis.crypto. I added a line to the class docs pointing readers at an import alias when both are in scope. Happy to rename (e.g. MemoryCache) if you'd rather.

Merge order: #7265, then #7245 (the import switches to the stable indexed-heap path), then this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants