vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.11k stars 26.73k forks source link

RSC does not suspend when async query is cached #69628

Closed PaulWild closed 2 weeks ago

PaulWild commented 2 weeks ago

Link to the code that reproduces this issue

https://github.com/PaulWild/next-no-suspense

To Reproduce

  1. Start the application and load root page
  2. The async component will suspend correctly, showing I am suspended!!!!!! until loaded and extra processing is done
  3. Reload the page, the component no long suspends but blocks the loading of the page until the component has finished rendering

Note - when using 'disable cache' in dev tools this works as expected. Additionally it looks like it might be fixed in v15, but Im hoping that a back fix might be deemed ok.

Current vs. Expected behavior

Expected: This should always suspend. In the example you can see I have slowed the component down by logging in a loop. In reality this is an issue for us as we have async components in a layout file, when they do not suspend they adversly effect our web scores, increasing time to first byte etc.

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:30 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6000
  Available memory (MB): 32768
  Available CPU cores: 10
Binaries:
  Node: 18.17.0
  npm: 9.6.7
  Yarn: N/A
  pnpm: N/A
Relevant Packages:
  next: 14.2.7 // Latest available version is detected (14.2.7).
  eslint-config-next: 14.2.7
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.5.4
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Not sure

Which stage(s) are affected? (Select all that apply)

Vercel (Deployed)

Additional context

No response

Harshkt214 commented 2 weeks ago

I can see this bug in the code you provided. However I tested it and fixed in the next15.0.0 canary release.

PaulWild commented 2 weeks ago

This is correct and I did mention that in the issue itself - however given that next15 and react19 are both in RC only and don't have stable releases I was hoping that back fixing this would be considered.

ztanner commented 2 weeks ago

Hi @PaulWild -- I believe this was fixed (albeit indirectly) by https://github.com/vercel/next.js/pull/64145. We made it so that force-dynamic also sets a default on any fetches on the page to not be cached.

Previously export const dynamic = "force-dynamic" was not influencing fetch cache behavior. So in this case, while the page was marked as dynamic, the fetch was cached. And in v14, fetches that are cached are currently buffered rather than streamed (this was also addressed in v15 via https://github.com/vercel/next.js/pull/68447)

If you update src/app/page.tsx to also have:

export const fetchCache = "force-no-store";

Then it'll behave as it does in Next 15 (ie, it won't cache any fetches, which is the intended behavior behind force-dynamic).

~However if you want to preserve fetch caching semantics while keeping force-dynamic in place, then I'm working on a backport here~:

ztanner commented 2 weeks ago

Hi @PaulWild, upon further investigation with the team, if your reproduction is reflective of what you're doing in your app, it's because it's doing work that's blocking the main thread causing the React server render to not be able to flush the initial HTML (which contains the loading state).

The reason it works on the first request is because the request isn't cached, so React sees the pending fetch, suspends, and then immediately flush the loading state to the client. On the second request, the fetch is cached, so the fetch immediately responds (so no need to suspend) but then the render is blocked by the loop you've created here. React will be stuck computing the work in this for loop before it can get a chance to flush.

Is there any way to move this work into an async task? If not, your best bet might be to disable fetch caching here as React will see the pending fetch and skip the main thread blocking work until after the HTML is flushed.

PaulWild commented 2 weeks ago

Hi @ztanner thanks for taking a look at this for us. Let me give a little more context that might explain some of the reproduction:

We are an ecommerce site. The problem area/components are in our header which we load in the layout.tsx. My thinking around adding to the export const dynamic = "force-dynamic" was to indicate that our header is opted out of static generation because we read from the headers. In retrospect maybe a detail that wasn't required.

In terms of how realistic the example is, it's not 100% as I think my example implies that we are doing some expensive calculation which isn't true. One example of what we are doing is loading our category structure to populate our aisles menu. The components are more akin to

const problemComponent = async () => {
    const someChangeableButCachableData = await fetch(data);
    const slightyProcessedData = minimalTransformation(someChangeableButCachableData)

    return ( 
     <FairlyComplexJsx data={slightyProcessedData} />
    )
}

An async task would make sense for an expensive calculation, but this feels more like a standard react component, maybe with a bigger structure than usual.. The difference might not be massive but it is impactful as it is currently effecting our web vital score. It can take a time to first byte from a reasonable 1-200ms to plus 1 seconds.

On the second request, the fetch is cached, so the fetch immediately responds but then the render is blocked by the loop you've created here. React will be stuck computing the work in this for loop before it can get a chance to flush.

This to me is the slightly surprising behaviour as I would have expected the look up to the data-cache to be fast but still involve a pending task and that initial flush.

PaulWild commented 2 weeks ago

That was unfortunate timing, having the metion of the backport fix being crossed out as I was replying. My hopes were a little higher before seeing that :D

ztanner commented 2 weeks ago

Hey @PaulWild -- I crossed out the backport primarily because the backport didn't fix the issue. I tried it against your repro and it still behaved the same way. Sorry for the false hope there!

This to me is the slightly surprising behaviour as I would have expected the look up to the data-cache to be fast but still involve a pending task and that initial flush.

Indeed, though this is a current limitation of React. I think you're looking for a future Suspense capability which is Suspense for CPU bound trees. At the moment Suspense is primarily optimized for IO (things like data fetches). There's an experimental property but it's only in Fiber (React's client renderer) and not Fizz (the streaming server renderer). Until then there's not much I can think of to do without tricking React into flushing the server HTML earlier. For example, if you add a await new Promise((resolve) => setTimeout(resolve, 500)); before your fetch, that'll give it enough time to flush the initial HTML while streaming the computationally expensive work.

ztanner commented 2 weeks ago

Also just to add another option... you might be able to leverage ISR to pre-generate the page at build time. And then revalidate the page when you know the data is changed (or on an interval) to signal when the page should be updated. That'll ensure the page is always a cache HIT without needing to perform any work when the user requests the page as the work will have already been performed by the time the request comes in.

PaulWild commented 2 weeks ago

Thanks for the advice! We will have to have a think about how we might optimise things. My mind also went straight to a promise with a setTimeout so very glad you suggested that too! It may actually improve perceived speed.

I did go back to my v15 version and yeah I realised that it wasn't working as originally thought. I think the force-dynamic was overriding the cache directive.

ztanner commented 2 weeks ago

Sounds good -- yeah, I think the handling in 15 is just masking the behavior because the dynamic rendering is forcing the fetch to hit the origin; React will see the pending data fetch and flush the HTML before even getting to the for loop. We're triggering the same behavior with the await setTimeout but with the added benefit of not actually opting out of the fetch caching behavior.

Going to close this for now as I don't believe this is actionable on our side, but I'm still happy to answer any questions or provide additional details about my previous suggestions. I've relayed this issue to the React team in case we can get a fix there that's backportable to v14.

github-actions[bot] commented 2 days ago

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.