Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

304 breaks local caching. #2050

Closed
thruflo opened this issue Nov 26, 2024 · 7 comments
Closed

304 breaks local caching. #2050

thruflo opened this issue Nov 26, 2024 · 7 comments

Comments

@thruflo
Copy link
Contributor

thruflo commented Nov 26, 2024

In Electric.Plug.ServeShapePlug, specifically here https://github.com/electric-sql/electric/blob/main/packages/sync-service/lib/electric/plug/serve_shape_plug.ex#L359, we check the etag of a request and return 304 if it matches:

conn
|> send_resp(304, "")
|> halt()

This doesn't set any caching headers. So ... the response comes back as max-age=0, private, must-revalidate:

curl -v 'http://localhost:3000/v1/shape?table=todos&offset=0_0&handle=69883540-1732638738539'
> ...
< HTTP/1.1 304 Not Modified
< date: Tue, 26 Nov 2024 16:42:15 GMT
< vary: accept-encoding
< cache-control: max-age=0, private, must-revalidate

This breaks local caching in the browser, which breaks offline support for Electric apps. Looking at the request in the Chrome dev tools, it comes through as 200 rather than a 304 (which is confusing, seems to be a Chromium bug https://issues.chromium.org/issues/40205097) but it is the 304 response and the cache-control header has max-age 0, must revalidate, so isn't cached in the local file cache:

image

I came across this because I noticed that it was breaking offline support:

image

To reproduce this you have to actually get the 304 request. This is returned when the browser re-fetches a URL after the local file cache has expired. So then re-fetching successfully when online gets a 304, which is then never cached so the offline support only works for the initial request.

Now ... the duration of the cache-control headers is very different for requests with content:

image

Vs requests that just have an up-to-date message:

image

Unfortunately because the up-to-date message is needed to trigger rendering of the content, this means that the content that's been loaded and is cached locally can't be used because the client still needs an up-to-date in order to display it.

So:

  1. presumably we need to set cache-control headers on a 304?
  2. can we set a longer expiry on the up-to-date? Because it is the weakest link for expiry of content for offline support. Or if not, what's out strategy here?
@KyleAMathews
Copy link
Contributor

Irrespective of the 304 issue — what's your goal here? That useShape works completely offline?

@thruflo
Copy link
Contributor Author

thruflo commented Nov 26, 2024

My specific goal when stumbling into it was making an offline capable web app (which you can reload in the browser and works because the data fetched is in the browser cache -- which works 💯 when the requests are cached but obviously fails when any request in the sequence falls out of the cache).

More generally, this impacts the proposition of "because Electric is just HTTP and data loading is cached, it comes out of the local browser cache / local edge cache / any cache". So the idea that you can make resilient apps that tolerate downtime without having to implement actual local persistence because the requests are just naturally cached. And the idea that you can navigate back to a page you visited before and you won't get a loading spinner.

In short:

  1. if a 304 isn't cached, we need to go re-fetch the data; this impacts offline support, not refetching when you revisit a page, etc.
  2. if an up-to-date response expires, it compromises all the other data that has a longer expiry but can no longer be rendered

@KyleAMathews
Copy link
Contributor

ah ok — so what I think we want here is that a 304 should include cache-control headers to extend the original cache. Probably the same max-age as the original cache. Or whatever the dynamically set max-age is once we have #1447 to calculate that.

The problem now is that original cache expires and we never extend it due to not setting a max-age on the 304s.

That solves 1.

For 2 — this seems like a client or end-user configuration problem — we do want fresh data by default and should only fall back on cached data if we know we're offline. There's no easy way to know we're offline or what the state of the cached data is w/o some sort of timeout on fetched responses, etc.

But ultimately, I don't think we should expect offline to work with the raw browser http cache — we don't control it and can't introspect on it. We have no idea if the cache is complete, etc. I think if people want actual offline support then they need to use indexdb/sqlite/pglite/etc. which can control the cache and reason about it.

@thruflo
Copy link
Contributor Author

thruflo commented Nov 26, 2024

I’m not sure re 2. The thing is that the ability of the browser to cache responses is very powerful and works for free for people out of the box. So it’s a strong incremental adoption thing that aligns with our positioning and differentiation.

To flip the question around, what are the drivers for not caching data you’ve received? Why can’t we support caching responses for a decent length of time?

The point about wanting fresh data: there’s nothing about keeping the data you have synced cached that precludes the client from constantly trying to fetch fresh data. That’s the current behaviour — it keeps trying to connect.

When it comes to why sync not fetch, the basic affordance of “navigate back to a previously viewed route” -> “look ma no loading spinner” is a big deal. The beauty is that we can deliver it without any of the complexity or code changes required to implement a local persistent store. So it feels worthwhile eyeballing the caching duration and up-to-date stuff to see if we can achieve it.

@KyleAMathews
Copy link
Contributor

Most apps don't want to immediately render old data and then a bit later render fresh data. It looks glitchy.

Ultimately it's the app developer who should choose how long to cache data for. So we can add that as an option to the http API. But keeping the default short is good as glitchy uis are annoying for most apps.

@thruflo
Copy link
Contributor Author

thruflo commented Nov 26, 2024

Yes, I can see there’s less control with this just-replay-the-requests approach.

I would love for there to be a best of all worlds solution — where the data is just there in the file cache and that doesn’t compromise UX.

I wonder if there might be some cleverness to add a little more control back in, like ignoring all but the last up-to-date message when detecting requests served from file cache.

I think my viewpoint is that once you bind a shape to a state variable with useShape, you are basically buying into a “render whatever has synced and update with fresh data when it comes in” approach. You can obviously force refetch with a custom param and you obviously don’t want to playback lots of flickering transactions.

But that is the UX you’d get with a persisted shape, so if we can provide the same UX without having to implement persistence that would be 🔥 (and is actually how it already works, within the cache window).

@thruflo
Copy link
Contributor Author

thruflo commented Nov 27, 2024

Just for context here's a little video of some current behaviour:

Screen.Recording.2024-11-27.at.18.17.48.mp4

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants