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

Implement HTTP caching #3231

Open
jeswr opened this issue May 9, 2024 · 9 comments
Open

Implement HTTP caching #3231

jeswr opened this issue May 9, 2024 · 9 comments
Labels
enhancement New feature or request interceptors Pull requests or issues related to Dispatcher Interceptors

Comments

@jeswr
Copy link

jeswr commented May 9, 2024

This would solve...

Performance and robustness issues associated with fetching the same HTTP resource multiple times.

The implementation should look like...

Use an interceptor(?) to implement http caching using the Cache-Control header. I'm inclined to think that the Expires header should not be implemented as http 1.0 has extremely limited use.

Additional context

Originally raised in #3221.

@jeswr
Copy link
Author

jeswr commented May 9, 2024

I've looked into implementing this as an interceptor, and need some information in order to proceed:

  • Is there existing code for caching and cloning responses in the code base that can be used (for this I was expecting to be able to use logic from the web cache which seems to be implementing the Service Worker Spec)
  • Is rfc7234 implemented anywhere in the codebase (I cannot find any indication that it is). http-cache-semantics seems to be a good reference implementation of it if not, however it is under the BSD-2-Clause so @kornelski would need to give permission in order for code to be re-used in this library without copying the BSD licence over.

@metcoder95
Copy link
Member

Is there existing code for caching and cloning responses in the code base that can be used (for this I was expecting to be able to use logic from the web cache which seems to be implementing the Service Worker Spec)

Can you elaborate on what you have in mind?

Is rfc7234 implemented anywhere in the codebase (I cannot find any indication that it is). http-cache-semantics seems to be a good reference implementation of it if not, however, it is under the BSD-2-Clause so @kornelski would need to permit for code to be re-used in this library without copying the BSD licence over.

The closest implementation will be the fetch one but I don't believe it implements its full RFC-7234 spec, so this will be a new implementation for the interceptor.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 9, 2024

I have somehow interest to implement it...

@jeswr
Copy link
Author

jeswr commented May 9, 2024

Can you elaborate on what you have in mind?

I was trying to work out the easiest way of caching the response object in-memory would be given that I'm assuming it would be coming through with the body as a readable-stream that needs to be cloned - I was assuming that the Web Cache would already need to implement such behavior. I'll step out of discussions of implementation details now if someone else is willing to take over.

I have somehow interest to implement it...

@Uzlopak I'm very time poor so I'm more than happy to step back on my side! Note you'll probably want to implement rfc9111 which appears to obsolete rfc7234.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 9, 2024

I am also not very time "rich", but I already investigated some hours in reading into rfc 9111.

The only thing which was not clear to me, is how we ensure that we dont open a security issue :D

What I mean is:

In a browser context you have one user and fetch calls can be cached, as the cache is user scoped. In node we dont have users. So a http cache would be globally activated (?) in nodejs.

So we need to have something like setGlobalDispatcher but for caching? setGlobalHttpCache?

@mcollina
Copy link
Member

I would start having an interceptor or Agent that implement the caching protocol of HTTP, and then worry about how we enable it all the time.

I think it's possible to have it always enabled and not cache any private data: that's how cdn works after all. Before worrying about this, we need the implementation ;).

@Uzlopak
Copy link
Contributor

Uzlopak commented May 10, 2024

Ok, i will start tonight. ;)

@kornelski
Copy link

I've implemented http-cache-semantics by reading the RFC start to finish, and translating each paragraph into code.

I highly recommend that approach, because it results in good comments with references to their RFC sections (I wish I kept more of them in my code), and helps ensure that unobvious behaviors are not missed.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 10, 2024

Usually when I implement an RFC or any other spec I print it out. Then like you say, use the text for commenting the code. When I implemented a paragraph I mark it in the print out that it was implemented, too. Yellow marker for implemented, pink marker for has an issue and should be checked again. When the whole text is colored, then I know the spec is implemented completely ;).

@metcoder95 metcoder95 added the interceptors Pull requests or issues related to Dispatcher Interceptors label May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request interceptors Pull requests or issues related to Dispatcher Interceptors
Projects
None yet
Development

No branches or pull requests

5 participants