Add new caching infrastructure #153

dgryski · 2025-03-06T21:35:37Z

No description provided.

cee-dub

So far this is looking good! I want to submit my comments so far, though I still need to review internal/abi/fastly/httpcache_guest.go and integration_tests/httpcache/main.go

internal/abi/fastly/http_guest.go

fsthttp/response.go

fsthttp/request.go

cee-dub

Reviewed integration tests. Still have to read the full implementation.

integration_tests/httpcache/main.go

cee-dub · 2025-03-24T18:36:45Z

integration_tests/httpcache/main.go

+	return nil
+}
+
+func testRequestCollapseThree(ctx context.Context) error {


Is this collapsing or just normal caching behavior? Does the "slow backend on r mean r2 and r3 are outstanding requests on the key before r can be resolved and that's what makes it collapsing? If so, please add a comment to explain.

Yes. Although since wasm is single-threaded I bet the time.Sleep() ends-up blocking and will complete before r2 or r3 call Send.

And indeed yes this is exactly what is happening:

stdout | 840c6049 | service built with tinygo stdout | 840c6049 | uri= https://http-me.glitch.me/anything/1213559501/ stdout | 840c6049 | sending r1 stdout | 840c6049 | sleeping in AfterSend stdout | 840c6049 | done sleeping stdout | 840c6049 | sending r2 stdout | 840c6049 | sending r3

This test will need to be written to some channels and goroutines to ensure the timing I want to happen actually is.

Ok, so rewriting the test to use goroutines + channels etc and the call to r2.Send() just blocks. This is because when we yield in the time.Sleep() function to another goroutine which does a blocking hostcall, control never returns to the first goroutine to continue handling the function. Solving this means (I think) exposing a SendAsync() method on fsthttp.Request and moving away from "just write blocking calls -- the user will manage concurrency with goroutines" we had previously operating under.

I'm starting to think we may also want a SendWait(), but it sounds like that's already how Send() works in TinyGo today, even with a goroutine and a polling loop.

integration_tests/httpcache/main.go

cee-dub · 2025-03-24T18:48:57Z

integration_tests/httpcache/main.go

+
+	const headerUA = "user-agent"
+
+	r.CacheOptions.AfterSend = func(r *fsthttp.CandidateResponse) error {


Does this need to have a sleep to allow for/de-flake actual collapsing?

I've never had this test flake. Not sure if that means this is a bad test..

integration_tests/httpcache/main.go

cee-dub · 2025-03-24T18:55:01Z

integration_tests/httpcache/main.go

+		return fmt.Errorf("response.Vary()=%v, want %v", got, want)
+	}
+
+	if got, wantLen := resp.SurrogateKeys(), 10; len(got) < wantLen {


Where does 10 come from?

Random length to ensure it's "actually" a SHA checksum I guess? Copied from the javascript code.

Would love to figure out a correct provenance and make it obvious.

cee-dub · 2025-03-24T18:57:00Z

integration_tests/httpcache/main.go

+	return nil
+}
+
+func testAfterSendCandidateResponsePropertiesCached(ctx context.Context) error {


How important do we think this test case is, and what sources do we have to reconcile the issues?

Seems like a decent candidate for return ErrSkip.

integration_tests/httpcache/main.go

internal/abi/fastly/httpcache_guest.go

cee-dub · 2025-03-24T23:28:02Z

internal/abi/fastly/httpcache_guest.go

+//	;;; the required size is written by the host to `nwritten_out` and the `$buflen`
+//	;;; error is returned.
+//	;;;
+//	;;; At the moment, HTTP cache keys must always be 32 bytes.


WUT. How does this deal with cache keys over 32 bytes?! Seems like many URLs would be longer than that...

Looking at another part of the Rust SDK it seems like this cache key is a salted sha256 of the key, but I don't know if that's something specific to the Rust SDK or behaviour I need to match in Go. OTOH reading the Rust code it seems like the user is just supposed to set the correct cache key in the cache metadata struct with no verification.

My understanding of cache keys for Fastly cache in general is opaque bytes up to some size (like 1500?)

Still finding the comment about 32 bytes strange. Check with the team?

That doc comment comes directly from the witx file defining the Compute API. I'll see what I can find out.

Right! I didn't think it was from you. Curious about what they say.

fsthttp/response.go

internal/abi/fastly/httpcache_guest.go

…aryRule

cee-dub

Only nit-level items remain.

integration_tests/httpcache/main.go

cee-dub · 2025-03-26T05:30:45Z

integration_tests/httpcache/main.go

+			return fmt.Errorf("candidate has age=%v or err=%v, want 0, nil", age, err)
+		}
+
+		if ttl, err := r.TTL(); ttl != 3600 || err != nil {


internal/abi/fastly/httpcache_guest.go

cee-dub · 2025-03-26T05:35:54Z

internal/abi/fastly/httpcache_guest.go

+//	;;; the required size is written by the host to `nwritten_out` and the `$buflen`
+//	;;; error is returned.
+//	;;;
+//	;;; At the moment, HTTP cache keys must always be 32 bytes.


Still finding the comment about 32 bytes strange. Check with the team?

fsthttp/cache.go

…sure it worked

internal: add http cache hostcalls

180b7bc

dgryski force-pushed the dgryski/http-cache branch 10 times, most recently from 2dac925 to b9e2d5e Compare March 13, 2025 00:50

dgryski force-pushed the dgryski/http-cache branch 2 times, most recently from 58268e8 to c9d31f8 Compare March 14, 2025 19:42

fsthttp: add new caching infrastructure

b631a1c

dgryski force-pushed the dgryski/http-cache branch from c9d31f8 to b631a1c Compare March 18, 2025 17:15

cee-dub self-requested a review March 20, 2025 20:35

cee-dub reviewed Mar 21, 2025

View reviewed changes

dgryski added 3 commits March 24, 2025 15:26

fsthttp: use xCache(Hits)? instead of fastlyCache(Hits)?

8f0acc3

internal/abi/fastly: fix godoc for HTTPBody.Length

1cc1664

fsthttp: shouldUseGuestCache -> shouldUseGuestCaching

4b4f353

cee-dub requested changes Mar 24, 2025

View reviewed changes

cee-dub reviewed Mar 24, 2025

View reviewed changes

internal/abi/fastly/httpcache_guest.go Outdated Show resolved Hide resolved

cee-dub reviewed Mar 25, 2025

View reviewed changes

dgryski added 6 commits March 24, 2025 22:03

integration_tests/httpcache: fix typo in test name

8b9e1da

internal/abi/fastly: initialize http handles to invalid (-1)

989a1b0

fsthttp: typo

ecc153a

fsthttp: use existing surrogateKey constant

28f89ea

fsthttp: document times are in seconds

4367da3

integration_tests/httpcache: use != instead of reflect.DeepEqual

72cb6bb

dgryski force-pushed the dgryski/http-cache branch from ef4dd16 to 14a174d Compare March 25, 2025 22:58

dgryski added 7 commits March 25, 2025 15:58

fsthttp: tweak error returns

8b4d958

integration_tests/httpcache: avoid shadowing variable

6c71bdd

integration_tests/httpcache: fix error message on test failure

14a174d

fsthttp: remove race codition

678daa0

internal: DefaultMediumBufLen -> DefaultSmallBufLen for HTTPCacheGetV…

7c135d4

…aryRule

internal: preallocate buffers for HTTPCacheGetSuggestedCacheOptions

06368ef

internal: mention SendV3/SendAsyncV2 do not set X-Cache

f3a70d7

cee-dub approved these changes Mar 26, 2025

View reviewed changes

dgryski added 6 commits March 26, 2025 08:36

internal: make OverrideKey layout match rest of functions

c39aae2

integration_tests/httpcache: match more text in body transform to be …

16e108e

…sure it worked

fsthttp: add custom ErrCachingNotSupported error

5563b3f

fsthttp: remove commented-out code

858e4dd

fsthttp,integration_tests: Response.Cached() -> Response.FromCache()

2436095

integration_tests/httpcache: fix comment

531d1df

dgryski force-pushed the dgryski/http-cache branch from d1b069b to b16011f Compare March 28, 2025 23:37

fsthttp: remove boolean arg from CandidateResponse.SetUncacheable()

b16011f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new caching infrastructure #153

Add new caching infrastructure #153

dgryski commented Mar 6, 2025

cee-dub left a comment

cee-dub left a comment

cee-dub Mar 24, 2025

dgryski Mar 27, 2025

dgryski Mar 27, 2025

cee-dub Mar 27, 2025

cee-dub Mar 24, 2025

dgryski Mar 27, 2025

cee-dub Mar 24, 2025

dgryski Mar 24, 2025

cee-dub Mar 24, 2025

cee-dub Mar 24, 2025

cee-dub Mar 24, 2025

dgryski Mar 25, 2025

cee-dub Mar 25, 2025

cee-dub Mar 26, 2025

dgryski Mar 26, 2025

cee-dub Mar 26, 2025

cee-dub left a comment

cee-dub Mar 26, 2025

cee-dub Mar 26, 2025


		const headerUA = "user-agent"

		r.CacheOptions.AfterSend = func(r *fsthttp.CandidateResponse) error {

Add new caching infrastructure #153

Are you sure you want to change the base?

Add new caching infrastructure #153

Conversation

dgryski commented Mar 6, 2025

cee-dub left a comment

Choose a reason for hiding this comment

cee-dub left a 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

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

cee-dub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment