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

Route Cache Caches the underlying API calls as well! #64

Closed
sathishzakapps opened this issue Jul 29, 2024 · 22 comments
Closed

Route Cache Caches the underlying API calls as well! #64

sathishzakapps opened this issue Jul 29, 2024 · 22 comments
Assignees

Comments

@sathishzakapps
Copy link

sathishzakapps commented Jul 29, 2024

Hi @dulnan

I'm encountering an issue with version 3.3.0.

When using the Route Cache for the route ~/pages/product/[id].vue, it caches the underlying API calls as well. When rendering the page, I use two API calls: one to get content (/api/cms/product) and the actual product data (/api/productInfo?id=productId), using Redis as the cache base.

image

I can see three entries in the cache: one for the actual page and two for /api/cms/product and /api/productInfo.

I would like only the page to be cached. Is there any way to achieve this? It worked as expected in version 3.2.0.

Thanks in advance; any help would be greatly appreciated.

@dulnan
Copy link
Owner

dulnan commented Jul 29, 2024

That sounds very weird and unexpected. I will look into this now.

@dulnan dulnan self-assigned this Jul 29, 2024
@sathishzakapps
Copy link
Author

sathishzakapps commented Jul 29, 2024

Thank you @dulnan

To add to the above I have tried to avoid caching the api calls using the enabledForRequest but does not help.

image

@dulnan
Copy link
Owner

dulnan commented Jul 29, 2024

I am unable to reproduce this behaviour. I have created a new API route that does not call useRouteCache() at all, meaning it should not be cached. I then call this API using useFetch() on a page that calls useRouteCache() with isCacheable(). I only have one entry in the route cache, the page. Only when I explicitly call isCacheable() inside the API event handler do I see that its response has been cached.

If you say thatenabledForRequest does not work, then something super fishy must be going on: This method is called very early on in the request life cycle and if its returns false, then the cache is completely unavailable for any subsequent code... This makes no sense 🤔

Do you have other places where you use useRouteCache()? e.g. inside a server middleware?

@dulnan
Copy link
Owner

dulnan commented Jul 29, 2024

Okay just as I sent it, I was able to reproduce it. And... it's VERY weird what I'm seeing. This is impossible.

@sathishzakapps
Copy link
Author

@dulnan I think the Nitro webhook calls the cache on every request that it is making including the page renders and API calls. After all, Page and all API calls are go through Nitro's life cycle.. (Not sure I am blabbering, though).

@dulnan
Copy link
Owner

dulnan commented Jul 29, 2024

Yeah, I found the problem, look:

Page component

<script lang="ts" setup>
import { useFetch, useRequestEvent } from '#imports'

const event = useRequestEvent()

if (event) {
  event.context.foobar = 'TEST'
}

const { data } = await useFetch('/api/uncacheableApi')
</script>

API route

import { defineEventHandler } from 'h3'

export default defineEventHandler<{ data: number }>((event) => {
  console.log('Test: ' + event.context.foobar)
  return {
    data: Date.now(),
  }
})

This logs Test: TEST to the console. Apparently the context is shared (?!) for the entire request, including any API calls that are made during server side rendering. One of the things I did change in the latest release was using this exact event.context to add the cache helper, instead of putting it on the event directly. This explains why enabledForRequest does not work: The first time it's called, your method returns true, because that path is not starting with /api. But at that point it is now set in event.context and thus available for any and all API routes. Crazy.

I will do a fix now and release it asap.

@sathishzakapps
Copy link
Author

Thank you so much @dulnan. Understood whats happening :)

dulnan added a commit that referenced this issue Jul 29, 2024
Because this is shared for the duration of the entire SSR request, including calls made from components to API handlers
@dulnan
Copy link
Owner

dulnan commented Jul 29, 2024

I have just released 3.3.1, would you like to give it a try?

Also thank you very much for bringing this issue to my attention! I would probably not have noticed this for some time and seeing as this bug could result in very unexpected runtime behaviour, I'm glad I was able to fix it now.

@sathishzakapps
Copy link
Author

sathishzakapps commented Jul 29, 2024

@dulnan Thank you so much!!! that worked :) the quickest response I have ever got!

you are doing great help by writing this module, we understand how important the cache is, providing the Nuxt's performance.

However, I encountered the below error. while serving the cached response.

Serving cached route for path: /product/A-23958 {
  fullKey: '/product/A-23958'
}

[nuxt] [request error] [unhandled] [500] Cannot set headers after they are sent to the client
  at new NodeError (node:internal/errors:405:5)  
  at ServerResponse.setHeader (node:_http_outgoing:652:11)  
  at setResponseHeaders (./.output/server/chunks/runtime.mjs:2559:20)  
  at Object.handler (./.output/server/chunks/runtime.mjs:5423:7)  
  at ./.output/server/chunks/runtime.mjs:3063:31  
  at process.processTicksAndRejections (node:internal/process/task_queues:95:5)  
  at async Object.callAsync (./.output/server/chunks/runtime.mjs:5544:16)  
  at async Server.toNodeHandle (./.output/server/chunks/runtime.mjs:3329:7)

This issue should not related to the new release, as I can see the same issue in the previous version as well!

Kindly let me know if I can avoid this error?

@dulnan
Copy link
Owner

dulnan commented Jul 29, 2024

Sure, could you show me which other modules you have installed or nitro/h3 related libraries you use? I know for example that the h3-compression library is not compatible with this module due to the way the response is compressed.

@sathishzakapps
Copy link
Author

Surely,
image

and nitro config

image

@dulnan
Copy link
Owner

dulnan commented Jul 30, 2024

Could you try (if easily possible) to disable nuxt-security to see if it would resolve the issue?

@sathishzakapps
Copy link
Author

sathishzakapps commented Jul 30, 2024

@dulnan tried removing them but no luck. not sure whats happening, shall check if any of the code that we wrote causing this issue and come back here. If I need further help..

@sathishzakapps
Copy link
Author

sathishzakapps commented Jul 30, 2024

@dulnan Thank you so much!!! that worked :) the quickest response I have ever got!

you are doing great help by writing this module, we understand how important the cache is, providing the Nuxt's performance.

However, I encountered the below error. while serving the cached response.

Serving cached route for path: /product/A-23958 {
  fullKey: '/product/A-23958'
}

[nuxt] [request error] [unhandled] [500] Cannot set headers after they are sent to the client
  at new NodeError (node:internal/errors:405:5)  
  at ServerResponse.setHeader (node:_http_outgoing:652:11)  
  at setResponseHeaders (./.output/server/chunks/runtime.mjs:2559:20)  
  at Object.handler (./.output/server/chunks/runtime.mjs:5423:7)  
  at ./.output/server/chunks/runtime.mjs:3063:31  
  at process.processTicksAndRejections (node:internal/process/task_queues:95:5)  
  at async Object.callAsync (./.output/server/chunks/runtime.mjs:5544:16)  
  at async Server.toNodeHandle (./.output/server/chunks/runtime.mjs:3329:7)

This issue should not related to the new release, as I can see the same issue in the previous version as well!

Kindly let me know if I can avoid this error?

@dulnan found the issue, getting this error since I used the routeRules to set the no-cache header for all the routes.

So once after the response sent to the client from our cache the routeRules is trying to set no-cache header.

PS: We have not faced this issue in the version 3.2.0 when we use the same routeRules config.

image

@dulnan
Copy link
Owner

dulnan commented Jul 31, 2024

Oh I see. I did not think of this scenario. Generally I would advise against using route cache + route rules together, but in this case it would be a valid use case to add headers. I will check to see if it's possible.

@sathishzakapps
Copy link
Author

Thanks @dulnan

@dulnan
Copy link
Owner

dulnan commented Aug 1, 2024

I looked into it and once again it's such an annoying problem with the way h3/nitro are setup. For some context: As an author of a module like nuxt-multi-cache, it's very hard to implement route caching. There are only so many ways one can hook into the life cycle of the request. The problem now is that the part where I cache the response happens at the very end of the request. This is why the module is able to also cache the headers applied by your route rule initially and also stores them in the cache. However, in the second request, when the module wants to serve something from cache, the only "sane" way to achieve that is inside the onRequest hook. There I load it from cache and immediately return the response. However: Nitro will still go through the entire rest of the request, calling hooks, applying route rules, etc. As of now I have not found a way to prevent that reliably.

That was also the case pre 3.3.0, however there it worked because "serving from cache" was implemented as an "event handler", because nitro did not provide a hook at that time. But in order to implement "stale if error" and "stale while revalidate", I could no longer implement it as an event handler, so I had to switch to the hook.

Anyway, as a temporary workaround, I suggest to not use routeRules to apply headers but instead apply them in a server middleware using setResponseHeader. In the meantime I will continue to explore options to make route rules work with route cache.

@sathishzakapps
Copy link
Author

Thank you for the detailed explanation @dulnan. For now I shall write the no-cache to the routes that I am not caching at all and I'll see if I need them in the routes I am caching and add them as necessary...

I need to understand more on h3/nitro implementation.

Thank you once again for the prompt replies :) helped me a lot..

@dulnan
Copy link
Owner

dulnan commented Aug 1, 2024

I may have found a workaround which I've already implemented in #66, but I still need to do some more testing. You may not need to apply the workaround for long, if it turns out to work properly!

@sathishzakapps
Copy link
Author

Great to hear! Hope all will be good (y) in the new PR and works well!!

@dulnan
Copy link
Owner

dulnan commented Aug 25, 2024

@sathishzakapps I just merged the change that should hopefully fix this issue. I will do some more testing (together with other fixes pushed today) and if all works, create a new release today.

@sathishzakapps
Copy link
Author

@dulnan Thank you for the update, I shall get the latest and test them and let you know in case of any issues? Once again thank you!!!

@dulnan dulnan closed this as completed Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants