feat: sqlite cache store

[flakey5]

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

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

cc @mcollina @ronag

[mcollina]

Can you add some docs?

[mcollina]

lgtm

[mcollina]

lgtm

[ronag]

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

[ronag]

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

[flakey5]

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]

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]

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]

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 ?

[metcoder95]

do we also support arrays?

[flakey5]

Of cache keys? Currently no

[ronag]

Why not make the database primary key a hash of url + method + vary headers?

[flakey5]

Why not make the database primary key a hash of url + method + vary headers?

That would still lead to the same state that I talked about here #3657 (comment)

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

[mcollina]

CI is very red :(

[mcollina]

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 ?

I think this would do it:

PRAGMA page_count; PRAGMA page_size;

Then do page_count * page_size.

[mcollina]

Why not make the database primary key a hash of url + method + vary headers?

@ronag I think this is possible, but I would likely investigate it after this lands. I think we would need to bring in https://www.npmjs.com/package/safe-stable-stringify to be able to have "stable" JSONs.

[ronag]

Why not make the database primary key a hash of url + method + vary headers?

@ronag I think this is possible, but I would likely investigate it after this lands. I think we would need to bring in https://www.npmjs.com/package/safe-stable-stringify to be able to have "stable" JSONs.

What's wrong with e.g:

function makeKey({ method, origin, path, vary }) { const hasher = crypto.createHash('sha256') hasher.update(origin.toLowerCase()) hasher.update(method.toLowerCase()) hasher.update(path ?? '') hasher.update(vary?.map(x => x.toLowerCase()).sort().join(',') ?? '') return hasher.digest('hex') }

[mcollina]

That would be very slow unfortunately :(. Unless we really need it.

[ronag]

@mcollina I would still argue that using a hash id would be a better solution. I think the performance overhead is negligible:

It can be done in 284.36 ns/iter which I would argue is probably similar to the overhead of doing a sql lookup of non-primary key (and also the extra lookup for our "upsert" in write).

e.g.

import { run, bench } from 'mitata' import crypto from 'crypto' import xxhash from 'xxhash-wasm' const xxHasher = await xxhash() const origin = 'https://example.com' const method = 'GET' const path = '/foo/bar' const vary = ['Accept', 'Accept-Encoding'] function generateKey () { let key = '' key += origin.toLowerCase() + '\uFFFF' key += method.toLowerCase() + '\uFFFF' key += (path ?? '') + '\uFFFF' if (vary?.length) { for (const v of vary.map(x => x.toLowerCase()).sort()) { key += v + ',' } } return key } bench('sha256', () => { return crypto.createHash('sha256').update(generateKey()).digest().readIntBE(0, 4) }) bench('xxhash', () => { return xxHasher.h32(generateKey()) }) run()

Though we would need to add xxhash-wasm as a dependency :/

[mcollina]

@ronag I'm going to land this and open a fresh issue for the follow-up. I have an idea.