-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Added notes for cache request hostnames #18634
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
Conversation
Deploying cloudflare-docs with
|
| Latest commit: |
077b5f7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c8cc55dd.cloudflare-docs-7ou.pages.dev |
| Branch Preview URL: | https://tori-pcx6484-cache-hostname.cloudflare-docs-7ou.pages.dev |
|
Files with changes (up to 15)
|
| ::: | ||
|
|
||
| :::note | ||
| Do not use dynamic or invalid hostnames in cache requests. This can cause DNS cache failures and latency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be more explicit. We should say that the hostname used in the cache request should match your hostname.
And then instead of "DNS cache failures" I'd say "a DNS lookup" which will significantly increase latency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if they use "request.headers.host" that'd be the correct hostname to use in all cases? I could be wrong about this though
EDIT: Err... we don't need to specify this since they'll just be passing the whole request object in.
I might say "Do not override the hostname in the request or use a hostname that differs from your own"?
irvinebroque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs more context — show vs. just tell
|
@mikenomitch @irvinebroque Thank you for the feedback! I updated the note with the details from Mike. An example could also be included if you think that's necessary to show it more explicitly? Let me know your thoughts. |
irvinebroque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as previously — show vs. tell
| When using the cache API, avoid overriding the hostname in cache requests, as this can lead to unnecessary DNS lookups and cache inefficiencies. Always use the hostname that matches the domain associated with your Worker. | ||
|
|
||
| ```js | ||
| async fetch(request, env, ctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation on this JS is a bit off. Everything within the brackets should be indented once.
Though actually, this also isn't valid fetch code. You would need to return the response from myCache.match(request) in order for it to be valid.
But... that doesn't really make sense because you might have a cache miss and you'd have to handle that (see this full code - https://developers.cloudflare.com/workers/examples/cache-api/)
I think since we don't want to show all of that code in here, what we should do is just drop the fetch() bits completely? It might be a tad less clear where request is coming from in this case, but I think it's fine.
So I'd just do
// avoid overriding the URL in request or passing a new request into the cache API
request.url = "https://some-overridden-value.com/";
let myCache = await caches.open('custom:cache');
let response = await myCache.match(request);
If it'll never work then I'd add this above the last line "// will never work because URL doesn't match Worker's domain"
mikenomitch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
|
@irvinebroque Ready for your re-review. I believe I need your approval as well for this to be unblocked. Thank you! |
irvinebroque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should suggest what people should do instead?
Co-authored-by: Brendan Irvine-Broque <[email protected]>
|
Howdy and thanks for contributing to our repo. The Cloudflare team reviews new, external PRs within two (2) weeks. If it's been two weeks or longer without any movement, please tag the PR Assignees in a comment. We review internal PRs within 1 week. If it's something urgent or has been sitting without a comment, start a thread in the Developer Docs space internally. PR Change SummaryAdded notes regarding the use of valid hostnames in cache requests to prevent DNS cache failures and latency issues.
Modified Files
How can I customize these reviews?Check out the Hyperlint AI Reviewer docs for more information on how to customize the review. If you just want to ignore it on this PR, you can add the Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add |
@irvinebroque Makes sense. I updated it again. Ready for your review when you have a chance. Thank you! |
Added notes - Do not use dynamic or invalid hostnames in cache requests. This can cause DNS cache failures and latency.