feat: sqlite cache store #3657

flakey5 · 2024-10-01T06:19:14Z

Note: this is a draft since #3562 isn't landed. Until it is landed, this will be based off of that pr's branch. For the actual diff see flakey5/undici@flakey5/3231...flakey5:undici:flakey5/20240910/sqlite-cache-store

This relates to...

Adding client side http caching (#3562)

Rationale

Adds a sqlite cache store option given the --experimental-sqlite flag is provided

Changes

Features

Cache store using sqlite as a backend

Bug Fixes

n/a

Breaking Changes and Deprecations

n/a

Status

cc @mcollina @ronag

test/cache-interceptor/cache-stores.js

lib/cache/sqlite-cache-store.js

mcollina

Can you add some docs?

lib/cache/sqlite-cache-store.js

mcollina

lgtm

lib/cache/sqlite-cache-store.js

mcollina

lgtm

lib/cache/sqlite-cache-store.js

index.js

test/cache-interceptor/cache-stores.js

ronag

Unnecessarily complicated by locking.
How does it "prune" i.e. free space?
Why errorCallback?

ronag · 2024-11-18T06:42:00Z

IMHO I would think you could do this quite similar to the "simplified" memory store.

lib/cache/sqlite-cache-store.js

ronag · 2024-11-18T10:56:55Z

index.js

+  const SqliteCacheStore = require('./lib/cache/sqlite-cache-store')
+  module.exports.cacheStores.SqliteCacheStore = SqliteCacheStore
+} catch (_) {
+  // Do nothing


I think we should check for expect error

Basically verify that this errors with a certain error code, and if not rethrow the error

flakey5 · 2024-11-19T08:10:29Z

Why errorCallback?

Goes back to the discussion we were having in the original caching pr for how to handle errors, we decided to handle them silently so the request still goes through.

Here it's just being used for handling json parse errors, since those shouldn't ever happen I would be in favor of just throwing outright though

How does it "prune" i.e. free space?

It prunes the entire database of expired values when we come across one when looking up a key, we can make this more stricter though (i.e. run the purge query on every write), wdyt?

ronag · 2024-11-19T08:34:09Z

It prunes the entire database of expired values when we come across one when looking up a key, we can make this more stricter though (i.e. run the purge query on every write), wdyt?

When the database is full (we are missing maxSize) we should remove entries whether they are expired or not. I think we should have something like, while beyond maxCount or maxSize drop 10% of the oldest entries.

flakey5 · 2024-11-19T09:05:19Z

I omitted maxCount here since it would break if there were multiple processes using a db for cache

If we do want it, I think we might be able to get away with it being stored somewhere in the db and being modified only with a procedure.

ronag · 2024-11-19T09:31:59Z

If we do want it, I think we might be able to get away with it being stored somewhere in the db and being modified only with a procedure.

I believe sqlite has meta commands for that, @mcollina ?

ronag · 2024-11-19T11:07:15Z

lib/cache/sqlite-cache-store.js

+        } catch (err) {
+          if (this.#errorCallback !== undefined) {
+            this.#errorCallback(err)
+          }


IMHO If the error callback is just for this I would rather skip it. Would even argue we should crash here if this happens (it shouldn't). @mcollina?

ronag · 2024-11-19T11:08:02Z

lib/cache/sqlite-cache-store.js

+        const existingValue = findValue(key, true)
+        if (existingValue) {
+          // Updating an existing response, let's delete it
+          updateValueQuery.run(
+            JSON.stringify(stringifyBufferArray(body)),
+            value.deleteAt,
+            value.statusCode,
+            value.statusMessage,
+            JSON.stringify(stringifyBufferArray(value.rawHeaders)),
+            value.cachedAt,
+            value.staleAt,
+            value.deleteAt,
+            existingValue.id
+          )
+        } else {
+          // New response, let's insert it
+          insertValueQuery.run(
+            url,
+            key.method,
+            JSON.stringify(stringifyBufferArray(body)),
+            value.deleteAt,
+            value.statusCode,
+            value.statusMessage,
+            JSON.stringify(stringifyBufferArray(value.rawHeaders)),
+            value.vary ? JSON.stringify(value.vary) : null,
+            value.cachedAt,
+            value.staleAt,
+            value.deleteAt
+          )
+        }


Can't you just always just overwrite? Does it update and insert need to be separate ops?

I think so but I'm not sure how messy it would be compared to just having two queries

Does update fail if the value doesn't exist? Doesn't sql have an insert or update command?

Does update fail if the value doesn't exist

It fails in the sense that it noops.

Doesn't sql have an insert or update command?

I'm not sure how well insert or update would work here based on how we're checking if there's a already cached value though. The only reliable and fast way we have to compare two cached values is with their ids. Otherwise, we need to compare their url, method, and vary headers to see if they were the same. The url and method comparisons are simple and can be done in sql, but comparing the vary headers is more difficult since it's stringified json. We can't compare it in sql since we don't know if the properties are in the same order, meaning we need to parse then compare (like what #findValues does).

I was experimenting with hashing the vary headers (sort properties alphabetically -> stringify -> crypto.createHash), however, it was a lot more complicated than just having two separate queries. It also would've only benefited writes, for reads we would still need to fetch each response and compare the vary headers like we already do

Co-authored-by: Robert Nagy <[email protected]> Co-authored-by: Isak Törnros <[email protected]> Signed-off-by: flakey5 <[email protected]>

lib/cache/sqlite-cache-store.js

ronag · 2024-11-19T20:55:49Z

lib/cache/sqlite-cache-store.js

+ * } & import('../../types/cache-interceptor.d.ts').default.CacheValue} SqliteStoreValue
+ */
+class SqliteCacheStore {
+  #maxSize = Infinity


this needs maxEntrySize as well?

ronag · 2024-11-19T20:57:14Z

lib/cache/sqlite-cache-store.js

+    const now = nowAbsolute()
+    for (const value of values) {
+      if (now >= value.deleteAt && !canBeExpired) {
+        this.#deleteExpiredValuesQuery.run(now)


Can this be a slow query? Should you just do a delete with id?

Depends, although indexed, as we are doing a range-like query it might hurt on the index lookup.

Have we considered maybe something like an LRU instead?

ronag · 2024-11-19T20:58:12Z

types/cache-interceptor.d.ts

+    /**
+     * Whether or not the cache is full and can not store any more responses
+     */
+    isFull?: Readonly<boolean>


I still don't see the point of this. The cache is never full. If it's full then the older values should be removed instead of not caching the most recent value.

metcoder95 · 2024-11-19T21:46:19Z

docs/docs/api/CacheStore.md

+
+### `SqliteCacheStore`
+
+The `SqliteCacheStore` stores the responses in a SQLite database.


Shall we add a note for the node:sqlite dependency in case someone attempts to use it within Node.js <= 22?

metcoder95 · 2024-11-19T21:50:05Z

lib/cache/sqlite-cache-store.js

+   * @returns {import('../../types/cache-interceptor.d.ts').default.GetResult | undefined}
+   */
+  get (key) {
+    if (typeof key !== 'object') {


do we also support arrays?

metcoder95 · 2024-11-19T21:53:55Z

lib/cache/sqlite-cache-store.js

+   * @returns {Writable | undefined}
+   */
+  createWriteStream (key, value) {
+    if (typeof key !== 'object') {


We should check that at least the minimum required properties are there; otherwise it can lead to confusing errorrs

metcoder95 · 2024-11-19T21:59:54Z

lib/cache/sqlite-cache-store.js

+    const now = nowAbsolute()
+    for (const value of values) {
+      if (now >= value.deleteAt && !canBeExpired) {
+        this.#deleteExpiredValuesQuery.run(now)


Depends, although indexed, as we are doing a range-like query it might hurt on the index lookup.

Have we considered maybe something like an LRU instead?

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from c33c78e to 6c8ea8f Compare October 1, 2024 06:20

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from 6c8ea8f to a015bf2 Compare October 15, 2024 02:16

flakey5 marked this pull request as ready for review October 15, 2024 02:17

flakey5 commented Oct 15, 2024

View reviewed changes

test/cache-interceptor/cache-stores.js Outdated Show resolved Hide resolved

SukkaW reviewed Oct 15, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Outdated Show resolved Hide resolved

mcollina requested changes Oct 15, 2024

View reviewed changes

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch 2 times, most recently from 592ffb3 to d951470 Compare October 16, 2024 07:21

mcollina requested changes Oct 16, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Outdated Show resolved Hide resolved

lib/cache/sqlite-cache-store.js Outdated Show resolved Hide resolved

lib/cache/sqlite-cache-store.js Show resolved Hide resolved

mcollina approved these changes Oct 25, 2024

View reviewed changes

mcollina reviewed Oct 25, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Outdated Show resolved Hide resolved

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from 19b0eaf to 93a306c Compare November 2, 2024 08:20

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch 2 times, most recently from 34b34d7 to 60a9679 Compare November 14, 2024 04:44

flakey5 requested a review from mcollina November 14, 2024 04:45

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from 60a9679 to 8e0fa89 Compare November 14, 2024 05:53

mcollina approved these changes Nov 14, 2024

View reviewed changes

mcollina reviewed Nov 14, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Outdated Show resolved Hide resolved

index.js Show resolved Hide resolved

test/cache-interceptor/cache-stores.js Outdated Show resolved Hide resolved

mcollina mentioned this pull request Nov 14, 2024

Unflag node:sqlite nodejs/node#55854

Closed

ronag requested changes Nov 18, 2024

View reviewed changes

ronag reviewed Nov 18, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Outdated Show resolved Hide resolved

ronag reviewed Nov 18, 2024

View reviewed changes

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch 2 times, most recently from bf40ed4 to 70fc6d3 Compare November 19, 2024 08:03

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from 70fc6d3 to dd7ba9a Compare November 19, 2024 08:18

ronag reviewed Nov 19, 2024

View reviewed changes

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from dd7ba9a to 89ed094 Compare November 19, 2024 18:13

feat: sqlite cache store

afa5d70

Co-authored-by: Robert Nagy <[email protected]> Co-authored-by: Isak Törnros <[email protected]> Signed-off-by: flakey5 <[email protected]>

flakey5 force-pushed the flakey5/20240910/sqlite-cache-store branch from 89ed094 to afa5d70 Compare November 19, 2024 20:48

ronag reviewed Nov 19, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Show resolved Hide resolved

ronag reviewed Nov 19, 2024

View reviewed changes

lib/cache/sqlite-cache-store.js Show resolved Hide resolved

ronag reviewed Nov 19, 2024

View reviewed changes

metcoder95 reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sqlite cache store #3657

feat: sqlite cache store #3657

flakey5 commented Oct 1, 2024 •

edited

Loading

mcollina left a comment

mcollina left a comment

mcollina left a comment

ronag left a comment

ronag commented Nov 18, 2024

ronag Nov 18, 2024

flakey5 Nov 19, 2024

mcollina Nov 19, 2024

flakey5 commented Nov 19, 2024

ronag commented Nov 19, 2024

flakey5 commented Nov 19, 2024

ronag commented Nov 19, 2024

ronag Nov 19, 2024

ronag Nov 19, 2024

flakey5 Nov 19, 2024

ronag Nov 19, 2024

flakey5 Nov 19, 2024

ronag Nov 19, 2024

ronag Nov 19, 2024

metcoder95 Nov 19, 2024

ronag Nov 19, 2024

metcoder95 Nov 19, 2024

metcoder95 Nov 19, 2024

metcoder95 Nov 19, 2024

metcoder95 Nov 19, 2024


		### `SqliteCacheStore`

		The `SqliteCacheStore` stores the responses in a SQLite database.

feat: sqlite cache store #3657

Are you sure you want to change the base?

feat: sqlite cache store #3657

Conversation

flakey5 commented Oct 1, 2024 • edited Loading

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

ronag left a comment

Choose a reason for hiding this comment

ronag commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flakey5 commented Nov 19, 2024

ronag commented Nov 19, 2024

flakey5 commented Nov 19, 2024

ronag commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flakey5 commented Oct 1, 2024 •

edited

Loading