Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cache/unstable): TtlCache #5662

Merged
merged 11 commits into from
Aug 21, 2024
Merged

feat(cache/unstable): TtlCache #5662

merged 11 commits into from
Aug 21, 2024

Conversation

lionel-rowe
Copy link
Contributor

@lionel-rowe lionel-rowe commented Aug 9, 2024

Closes #5669

@lionel-rowe lionel-rowe requested a review from kt3k as a code owner August 9, 2024 03:23
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (99200dc) to head (37572e5).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5662      +/-   ##
==========================================
- Coverage   96.28%   96.25%   -0.03%     
==========================================
  Files         474      475       +1     
  Lines       38383    38475      +92     
  Branches     5578     5587       +9     
==========================================
+ Hits        36957    37035      +78     
- Misses       1384     1398      +14     
  Partials       42       42              

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

@iuioiua
Copy link
Collaborator

iuioiua commented Aug 9, 2024

Please see #4608 (comment)

@iuioiua iuioiua self-requested a review August 19, 2024 05:30
cache/ttl_cache.ts Outdated Show resolved Hide resolved
cache/ttl_cache.ts Outdated Show resolved Hide resolved
cache/ttl_cache.ts Outdated Show resolved Hide resolved
cache/ttl_cache.ts Outdated Show resolved Hide resolved
cache/ttl_cache.ts Outdated Show resolved Hide resolved
cache/ttl_cache.ts Outdated Show resolved Hide resolved
cache/ttl_cache_test.ts Show resolved Hide resolved
@lionel-rowe lionel-rowe force-pushed the ttl-cache branch 4 times, most recently from 52c7efc to 3769753 Compare August 20, 2024 16:12
cache/ttl_cache_test.ts Show resolved Hide resolved
@iuioiua iuioiua changed the title feat(cache/unstable): add TTL (time-to-live) cache feat(cache/unstable): TtlCache Aug 21, 2024
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL, @kt3k.

cache/ttl_cache.ts Outdated Show resolved Hide resolved
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you.

cache/ttl_cache.ts Outdated Show resolved Hide resolved
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM when Asher's point addressed

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I removed [Symbol.toStringTag](). We don't implement this anywhere else in std. Either way, thank you for this PR!

@iuioiua iuioiua enabled auto-merge (squash) August 21, 2024 10:42
@iuioiua iuioiua merged commit 2a0f511 into denoland:main Aug 21, 2024
16 checks passed
Copy link
Contributor

@jsejcksn jsejcksn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to document the valid range for TTL values in this implementation. Because it defers to setTimeout, this will be implementation-dependent (see the spec), but testing can be performed to inform a suggestion for Deno at least. Here is an example demonstrating that the current build of V8 in Deno only properly handles unsigned 32-bit integers:

settimeout_range.test.ts:

import { AssertionError } from "jsr:@std/assert@1.0.3/assertion-error";

console.log("Deno.version", Deno.version);

async function assertIntervalIsScheduledAfter1Millisecond(
  interval: number,
): Promise<void> {
  let timestamp: number | undefined;
  const expectedTimeStamp = Date.now() + interval;

  const timerId = setTimeout(() => {
    timestamp = Date.now();
  }, interval);

  await new Promise((resolve) => setTimeout(resolve, 1));

  if (timestamp) {
    throw new AssertionError(
      `The callback completed ${
        expectedTimeStamp - Date.now()
      } ms earlier than expected.`,
    );
  }

  clearTimeout(timerId);
}

Deno.test("setTimeout(): Schedules intervals…", async (t) => {
  await t.step(
    "less than 2 ** 31",
    () => assertIntervalIsScheduledAfter1Millisecond(2 ** 31 - 1),
  );

  await t.step(
    "greater than or equal to 2 ** 31",
    () => assertIntervalIsScheduledAfter1Millisecond(2 ** 31),
  );
});

Test output:

% deno test settimeout_range.test.ts
------- pre-test output -------
Deno.version { deno: "1.46.1", v8: "12.9.202.2-rusty", typescript: "5.5.2" }
----- pre-test output end -----
running 1 test from ./settimeout_range.test.ts
setTimeout(): Schedules intervals… ...
  less than 2 ** 31 ... ok (3ms)
  greater than or equal to 2 ** 31 ... FAILED (1ms)
setTimeout(): Schedules intervals… ... FAILED (due to 1 failed step) (4ms)

 ERRORS 

setTimeout(): Schedules intervals… ... greater than or equal to 2 ** 31 => ./settimeout_range.test.ts:34:10
error: AssertionError: The callback completed 2147483647 ms earlier than expected.
		throw new AssertionError(
		      ^
    at assertIntervalIsScheduledAfter1Millisecond (file:///Users/deno/settimeout_range.test.ts:18:9)
    at eventLoopTick (ext:core/01_core.js:213:9)
    at async innerWrapped (ext:cli/40_test.js:191:5)
    at async exitSanitizer (ext:cli/40_test.js:107:27)
    at async Object.outerWrapped [as fn] (ext:cli/40_test.js:134:14)
    at async TestContext.step (ext:cli/40_test.js:492:22)
    at async file:///Users/deno/settimeout_range.test.ts:34:2

 FAILURES 

setTimeout(): Schedules intervals… ... greater than or equal to 2 ** 31 => ./settimeout_range.test.ts:34:10

FAILED | 0 passed (1 step) | 1 failed (1 step) (5ms)

error: Test failed

*
* @experimental **UNSTABLE**: New API, yet to be vetted.
*
* Automatically removes entries once the configured amount of time elapses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more clear to use the word "after" (this is the word used in the spec) since the spec doesn't seem to enforce that a runtime must prioritize timer tasks over all other tasks in the event loop — which essentially means that a runtime is free to delay execution of the callback for an unbounded amount of time.

Suggested change
* Automatically removes entries once the configured amount of time elapses.
* Automatically removes entries after the configured amount of time elapses.

@iuioiua
Copy link
Collaborator

iuioiua commented Aug 26, 2024

@jsejcksn, would you be able to submit a PR containing your suggestions?

@jsejcksn
Copy link
Contributor

@jsejcksn, would you be able to submit a PR containing your suggestions?

@iuioiua Created: #5834

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: TtlCache addition to @std/cache
4 participants